Where communities thrive


  • Join over 1.5M+ people
  • Join over 100K+ communities
  • Free without limits
  • Create your own community
People
Repo info
Activity
G. Gibson
@mistergibson
ok ... sleep tight :)
G. Gibson
@mistergibson
@elia -- when you get up: When I exclude 'opal-parser' I don't get the error.
Ghost
@ghost~59874280d73408ce4f704e9c

@elia i think there is a bug in js inlining, sometimes not generating a necessary return
example opal-active support String.camelize
non-working, generated by 0.11:

return (Opal.defn(self, '$camelize', TMP_String_camelize_2 = function $$camelize(first_letter) {
      var self = this;

      if (first_letter == null) {
        first_letter = "upper";
      }
      self.$underscore().replace(/(^|_)([^_]+)/g, function(match, pre, word, index) {
      var capitalize = first_letter === "upper" || index > 0;
      return capitalize ? word.substr(0,1).toUpperCase()+word.substr(1) : word;
    })

manual workaround:

    return (Opal.defn(self, '$camelize', TMP_String_camelize_2 = function $$camelize(first_letter) {
      var self = this;

      if (first_letter == null) {
        first_letter = "upper";
      }
      return self.$underscore().replace(/(^|_)([^_]+)/g, function(match, pre, word, index) {
      var capitalize = first_letter === "upper" || index > 0;
      return capitalize ? word.substr(0,1).toUpperCase()+word.substr(1) : word;
    })
note the added return in the middle
original code:
  def camelize(first_letter = :upper)
    `#{underscore}.replace(/(^|_)([^_]+)/g, function(match, pre, word, index) {
      var capitalize = #{first_letter} === #{:upper} || index > 0;
      return capitalize ? word.substr(0,1).toUpperCase()+word.substr(1) : word;
    })`
  end
workaround:
  def camelize(first_letter = :upper)
    `return #{underscore}.replace(/(^|_)([^_]+)/g, function(match, pre, word, index) {
      var capitalize = #{first_letter} === #{:upper} || index > 0;
      return capitalize ? word.substr(0,1).toUpperCase()+word.substr(1) : word;
    })`
  end
Ghost
@ghost~59874280d73408ce4f704e9c
now, where would i have to look for fixing this in the compiler?
Ghost
@ghost~59874280d73408ce4f704e9c
or how can i debug such things?
Simon George
@sfcgeorge
That native inlining issue @janbiedermann mentions is reported here opal/opal-activesupport#16 - Another workaround is rather than adding a return you can just call .to_s on the backtick string. I don't know why that would change what is happening. Does it force a native value to be cast into an Opal value or something?
Elia Schito
@elia

@janbiedermann try adding a semicolon before closing the backticks, that said to debug this kind of stuff I often do

opal -cEO -e "def foo; `return 123, function(){}` end"

and

opal --sexp  -e "def foo; `return 123, function(){}` end"
Ghost
@ghost~59874280d73408ce4f704e9c
@sfcgeorge when you add the the .to_s the compiler adds the returnabove, thats why it works with .to_s
Simon George
@sfcgeorge
@janbiedermann Interesting. That almost makes it seem like expected behaviour. If you show you intend to use the value then it returns it. Seems unhelpful though, especially if you don't know what the return type will be.
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