Where communities thrive


  • Join over 1.5M+ people
  • Join over 100K+ communities
  • Free without limits
  • Create your own community
People
Repo info
Activity
Ghost
@ghost~59874280d73408ce4f704e9c
@elia adding a ;doesnt help
Ghost
@ghost~59874280d73408ce4f704e9c

@iliabylich changing https://github.com/opal/opal/blob/ee50768d368959b33a77b58028a90ec9bded64a1/lib/opal/compiler.rb#L436 to

          if multiline
            # xstr starts with interpolation
            # then it must contain js_return inside
            if first_child.type == :begin
              s(:js_return, sexp)
            else
              sexp
            end
          else
            if first_child.type == :str

helps the above troubled case, also tests pass, though i am not sure, if that is the right thing to do there, looks too specific, or would you be ok with that? then i would provide a PR

G. Gibson
@mistergibson
Question: is there any plan to support REXML in the StandardLib? I notice it is missing - compared to MRI.
Ghost
@ghost~59874280d73408ce4f704e9c

@elia @iliabylich found another issue, for:

      def native_state_changed?(next_state_hash)
        next_state = next_state_hash.to_n
        %x{
          var current_state = #{@native}.state

compiler generates:

    next_state = next_state_hash.$to_n();
    return
    var current_state = self["native"].state

which wont work. Now fixed it here https://github.com/opal/opal/blob/ee50768d368959b33a77b58028a90ec9bded64a1/lib/opal/compiler.rb#L446 with this:

            if first_child.type == :str
              old_value = first_child.children[0]
              if old_value.include?('return')
                # 'return' is already there
                sexp
              else
                second_child, *second_rest_children = *rest_children
                if second_child && second_child.type == :str && !(old_value =~ /^\s*\n$/).nil? && !(second_child.children[0] =~ /^\s*var/).nil?
                  sexp
                else
                  first_child = s(:js_return, first_child)
                  sexp.updated(nil, [first_child, *rest_children])
                end
              end
works, tests work, but it looks so ugly. How can i make that better?
Ilya Bylich
@iliabylich
Opal injects return only for a single line x-strings. But the criteria for marking x-string as multiline is wrong - multiline = strs.any? { |str| str.end_with?(";\n") }
So if there are no lines ending with ; Opal decides that that's a single line x-string and injects a return if x-string doesn't have it
I guess correct multiline detection is simply strs.length > 1

I don't like the idea of mutating JS code, but we have to do it in cases like

list.map { |item| `item.toString()` }

In this case block becomes a JS function, so we have to inject explicit return

Ghost
@ghost~59874280d73408ce4f704e9c
right, i wasnt sure why that ;\nwas there, i am not a js coder ;) , i ll try your new multiline detection with my 2 cases
Ilya Bylich
@iliabylich
@janbiedermann Your solution checks for JS var keyword. What about other keywords? I guess it's not about var, it's about singleline/multiline x-strings
Ghost
@ghost~59874280d73408ce4f704e9c
i think so, that was just to fix this case, not really a 'solution'
Ilya Bylich
@iliabylich
Also there's another bug, if old_value.include?('return') # return is already there <- right here
def m
  `"return"`
end
Ghost
@ghost~59874280d73408ce4f704e9c
yes, so it get quickly ugly in this area
Ilya Bylich
@iliabylich
To handle such cases we need a JS parser :smile:
Ghost
@ghost~59874280d73408ce4f704e9c
yes, thats what i thought too, or maybe just a convention, like: inline blocks are executed as function and must 'return' a value, if so desired
Ilya Bylich
@iliabylich
I'm not sure about it, Opal generates anonymous utility functions in a couple of places. If Opal wraps your code into JS function you as a developer have to add explicit return (even if it's not obvious for Ruby developer).
For example:
a = if true
  `true`
else
  `false`
end
A rule like "Opal never adds 'return' to your x-strings" requires developers to write return true and return false
Ghost
@ghost~59874280d73408ce4f704e9c
maybe only for multiline blocks then?
Ilya Bylich
@iliabylich
That's the current behavior (after fixing an issue with multiline x-strings detection) :smile:
Ghost
@ghost~59874280d73408ce4f704e9c
just tried your strs.length > 1, fixes the latter case of the added return, but does not fix the first case of the missing return
makes 6 specs fail. Ok, a progress made, latter case fixed with beauty, ill digg on
Ilya Bylich
@iliabylich
Because it has a return inside
In the anonymous function
Wait, are you talking about opal/opal-activesupport#16 ?
Ghost
@ghost~59874280d73408ce4f704e9c
yes
Ilya Bylich
@iliabylich
Yes, that's it, similar to
def m
  `'return'`
end
# or
def m
  `(function() { return 1})()`
end
We can't fix it
Or even this :smile:
def m
  `#{"return"}`
end
Ghost
@ghost~59874280d73408ce4f704e9c
Then the fix is documentation, i guess: if you use anonymous functions with return in a inlined js block, you must explicitly return a value from the inlined block
Ghost
@ghost~59874280d73408ce4f704e9c
no wait, its worse, it would also hit a return_ticket_price = #{total_ticket_price - to_ticket_price}
Ghost
@ghost~59874280d73408ce4f704e9c

makes 6 specs fail

is actually not true, works perfectly with mulitline = strs > 0, was my debugging output leftover that made tests fail

Ghost
@ghost~59874280d73408ce4f704e9c

@iliabylich now this multiline patch breaks for example:

  def self.find(selector)
    `$(#{selector})`
  end

from opal-query, which is now considered a multiline, so no return is added, leading to:

  return Opal.defs(self, '$find', TMP_find_1 = function $$find(selector) {
    var self = this;

    $(selector)
  }, TMP_find_1.$$arity = 1)
chaning it to multiline = strs.any? { |str| str.end_with?("\n") }does seem to do the right thing
Simon George
@sfcgeorge
Kinda makes sense as semicolons are optional in JS, checking for newlines would do what it says on the tin I guess.
Ilya Bylich
@iliabylich
I guess it does handle
def m
  %x{
    1 + 1
  }
end
Simon George
@sfcgeorge
I'd assume there's a strip somewhere
Ilya Bylich
@iliabylich
That's an AST, not a string with the source code
Simon George
@sfcgeorge
Yeah, an AST usually doesn't have whitespace in
Ghost
@ghost~59874280d73408ce4f704e9c
but thats a ruby ast, where the inline js is just a string
or many strings
interrupted by ruby ast, if you js #{ ruby } more js
and the problem is exactly there, to understand js without parsing it :(
Simon George
@sfcgeorge
If the JS is just a string then surely you can strip it, so that wouldn't mistakenly be treated as multiline.
Ghost
@ghost~59874280d73408ce4f704e9c
and the ruby ast in between?
so its still ast nodes, of type :str, containing partial js
thats a :str
oh, ok, so for this case:
`$(#{selector})`