These are chat archives for opal/opal

17th
Jan 2018
Jan Biedermann
@janbiedermann
Jan 17 2018 16:44

@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
Jan 17 2018 18:05
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

Jan Biedermann
@janbiedermann
Jan 17 2018 18:09
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
Jan 17 2018 18:10
@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
Jan Biedermann
@janbiedermann
Jan 17 2018 18:11
i think so, that was just to fix this case, not really a 'solution'
Ilya Bylich
@iliabylich
Jan 17 2018 18:12
Also there's another bug, if old_value.include?('return') # return is already there <- right here
def m
  `"return"`
end
Jan Biedermann
@janbiedermann
Jan 17 2018 18:13
yes, so it get quickly ugly in this area
Ilya Bylich
@iliabylich
Jan 17 2018 18:13
To handle such cases we need a JS parser :smile:
Jan Biedermann
@janbiedermann
Jan 17 2018 18:15
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
Jan 17 2018 18:17
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
Jan Biedermann
@janbiedermann
Jan 17 2018 18:20
maybe only for multiline blocks then?
Ilya Bylich
@iliabylich
Jan 17 2018 18:21
That's the current behavior (after fixing an issue with multiline x-strings detection) :smile:
Jan Biedermann
@janbiedermann
Jan 17 2018 18:22
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
Jan 17 2018 18:24
Because it has a return inside
In the anonymous function
Wait, are you talking about opal/opal-activesupport#16 ?
Jan Biedermann
@janbiedermann
Jan 17 2018 18:24
yes
Ilya Bylich
@iliabylich
Jan 17 2018 18:25
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
Jan Biedermann
@janbiedermann
Jan 17 2018 18:38
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
Jan Biedermann
@janbiedermann
Jan 17 2018 18:47
no wait, its worse, it would also hit a return_ticket_price = #{total_ticket_price - to_ticket_price}
Jan Biedermann
@janbiedermann
Jan 17 2018 18:58

makes 6 specs fail

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

Jan Biedermann
@janbiedermann
Jan 17 2018 20:26

@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
Jan 17 2018 21:02
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
Jan 17 2018 21:19
I guess it does handle
def m
  %x{
    1 + 1
  }
end
Simon George
@sfcgeorge
Jan 17 2018 21:21
I'd assume there's a strip somewhere
Ilya Bylich
@iliabylich
Jan 17 2018 21:22
That's an AST, not a string with the source code
Simon George
@sfcgeorge
Jan 17 2018 21:26
Yeah, an AST usually doesn't have whitespace in
Jan Biedermann
@janbiedermann
Jan 17 2018 21:28
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
Jan 17 2018 21:32
If the JS is just a string then surely you can strip it, so that wouldn't mistakenly be treated as multiline.
Jan Biedermann
@janbiedermann
Jan 17 2018 21:32
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})`
there is a tree :xstr with children:
:str (first part of js) :begin (ruby ast) :str (last part of js)
that are :strs
Jan Biedermann
@janbiedermann
Jan 17 2018 21:40
so the 'mulitline' her is not the whitespace in the :strbut multiple :str, was, now with above fix, to look in the :str for a \nmulitline js is detected.
But @sfcgeorge you have a point, as minified js is minified to one line strings with no whitespace, but how would that stripping of whitespace benefit the situation here?