getting `Unexpected token '('. Expected a property name.` inside riot.js code #2002

Closed
o5k opened this Issue Sep 27, 2016 · 8 comments

Projects

None yet

4 participants

@o5k
o5k commented Sep 27, 2016 edited

The use of string.replace is giving different results on different browsers:

Describe your issue:

I get the following error

Unexpected token '('. Expected a property name.

inside riot.js on line 789

return new Function('E', expr + ';')    // eslint-disable-line no-new-func

which makes the website non usable (breaks loading any js code after arriving at that line).

Can you reproduce the issue?

I have translatable strings in my riot tags HTML as follow

<a>{ t('Homepage') }</a>

and

<a>{ t('Please choose from the __storeCount__ stores available', {storeCount: this.storeCount}) }</a>

with t being the translate function.

After a lot of investigation I found that the error happens exactly when a translatable string has parameters (second example that I mentioned).

During investigation, the param str (line 784 in riot.js) that the function _create receives has the following value

{t('Please choose from the __storeCount__ stores available', {storeCount: this.storeCount})}

It is the string to be used to reproduce the error and debug. I found out that expr gets a different value from the call of _getTmpl depending on the browser.
When run on Chrome iOS 47 (one of those on which I get the error), it has

try{return ("t"in this?this:window).t('Please choose from the __storeCount__ stores available',{("storeCount"in this ?this:window).storeCount:this.storeCount})}catch(e){E(e,this)}

and when run on desktop Chrome or Firefox for example (on which no error is thrown) it has

try{return ("t"in this?this:window).t('Please choose from the __storeCount__ stores available',{storeCount:this.storeCount})}catch(e){E(e,this)}

Going deeper, it is exactly the result of _wrapExpr that is different depending on the browser, more precisely at the level of expr = expr.replace(JS_VARNAME, function (match, p, mvar, pos, s) { which returns a different string. I created this jsfiddle that you can run on Chrome iOS 47 to get the broken string and on any normal browser (like latest chrome desktop) to get the correct string.

On which browser/OS does the issue appear?

I get this error only on iOS devices, on some specific Safari versions (on iOS 8, 9 and 10) and Safari UIWebView, but mainly using Chrome 47.0.2526.107 (which is the latest installable version for a couple of old iOS versions, and which is what makes this bug important).

Which version of Riot does it affect?

We noticed this error few months ago, and it affects several riot versions; for investigaing, and to have the same line numbers I mentioned, I'm using Riot 2.6.1

How would you tag this issue?

  • Question
  • Bug
  • Discussion
  • Feature request
  • Tip
  • Enhancement
  • Performance
@aMarCruz aMarCruz added the bug label Sep 27, 2016
@aMarCruz aMarCruz self-assigned this Sep 27, 2016
@aMarCruz
Member
aMarCruz commented Sep 28, 2016 edited

@o5k thanks. Looks like iOS regex implementation of lookahead is broken. The last version of tmpl includes a fix for another bug (using (?=:)) that, I guess, is causing the current one. I will take a look on this ASAP.

@o5k
o5k commented Oct 4, 2016 edited

@aMarCruz Thank you for the quick reply and for considering some time for this issue. Any news so far?

@trassmann

@aMarCruz Hi Alberto, I can confirm this bug and am also struggling with its repercussions. Any news on this?

@GianlucaGuarini
Member
GianlucaGuarini commented Oct 11, 2016 edited

I hope we will fix this issue soon but in the mean time if someone would like to help us with a pull requests it would be awesome https://github.com/riot/tmpl (It helps more than "news on this?")

@o5k
o5k commented Oct 13, 2016 edited

@aMarCruz @GianlucaGuarini I just found out the main source of the problem:

As shown above, the string that we pass to the function contains an object as follow {storeCount: this.storeCount}. What happens is that the function that creates the template _getTmpl considers both storeCount and this.storeCount as variables to check if they are available on this or on window.. It's like providing an object with { variable: variable } which results in a broken string.

For now, to fix the problem, I changed all the params in the translatable strings to use the double quotes for the attributes => {"storeCount": this.storeCount}. Like this the parser will not get confused, and the generated string will be correct.

I'm not sure if we can consider this the solution of the problem, or just a trick to avoid it and that it still has to be fixed (by making an extra check inside the function to detect keys and values of an object).

@aMarCruz aMarCruz added a commit to riot/tmpl that referenced this issue Oct 13, 2016
@aMarCruz aMarCruz Fix riot/riot#2002 6eb4989
@aMarCruz aMarCruz referenced this issue in riot/tmpl Oct 13, 2016
Merged

Merge Dev into Master #18

@aMarCruz
Member
aMarCruz commented Oct 13, 2016 edited

@o5k sorry by the delay. Really I don't know why this bug happens. I made a test with the exact expression, with Chrome 47.0.2526 on iPhone 6S (iOS 9.0.0), 6S Plus (iOS 9.0.1), and SE (iOS 9.3.0).

Too strange, the issue is present in iOS 9.3.0 only, looks like the implementation of RegExp comes from the OS and not from the browser.

tmpl_bug_2002

anyway, I just publish tmpl v2.4.2 which (I hope) fixes this issue, please test it.

cc: @GianlucaGuarini

@GianlucaGuarini
Member

@aMarCruz awesome, I will bump a new riot release soon

@o5k
o5k commented Oct 14, 2016

@aMarCruz yeah nice job 👍 I confirm that it fixed the issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment