New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ES6 template literals forward slashes issue #2167

Closed
jpoppe opened this Issue Dec 19, 2016 · 16 comments

Comments

Projects
None yet
6 participants
@jpoppe

jpoppe commented Dec 19, 2016

Help us to manage our issues by answering the following:

  1. Describe your issue:

Forward slashes are not processed properly in ES6 template strings and generate Uncaught SyntaxError: Unterminated template literal errors.

Works (regular string):
'http://riot.com/guide'

Does not work (template literal, mind the backticks):
`http://riot.com/guide`

  1. Can you reproduce the issue?
  1. On which browser/OS does the issue appear?
    Tested on latest Firefox/Chrome (Arch Linux)

  2. Which version of Riot does it affect?
    I've tested it with the latest 3.0.x release.

  3. How would you tag this issue?

  • Question
  • Bug
  • Discussion
  • Feature request
  • Tip
  • Enhancement
  • Performance

@jpoppe jpoppe changed the title from ES6 template literals backslashes issue to ES6 template literals forward slashes issue Dec 19, 2016

@cognitom

This comment has been minimized.

Show comment
Hide comment
@cognitom

cognitom Dec 20, 2016

Member

Hi @jpoppe. I think we still need to transpile ES6 into ES5, so it might has not be a issue, at this point.

Your tag was compiled like this:

riot.tag2('my-tag', '<h1>{message}</h1>', '', '', function(opts) {
    this.message = `http:
});

because riot-compiler is supporting //not a comment inside single or double quatations, but not in backticks.
https://github.com/riot/compiler/blob/v3.1.1/lib/brackets.js#L23

@aMarCruz we could add a backtick support like below. Is it realistic? I'm not sure about its side effects...

// current
var R_STRINGS = /"[^"\\]*(?:\\[\S\s][^"\\]*)*"|'[^'\\]*(?:\\[\S\s][^'\\]*)*'/g
// with backticks
var R_STRINGS = /"[^"\\]*(?:\\[\S\s][^"\\]*)*"|'[^'\\]*(?:\\[\S\s][^'\\]*)*'|`[^`\\]*(?:\\[\S\s][^`\\]*)*`/g
Member

cognitom commented Dec 20, 2016

Hi @jpoppe. I think we still need to transpile ES6 into ES5, so it might has not be a issue, at this point.

Your tag was compiled like this:

riot.tag2('my-tag', '<h1>{message}</h1>', '', '', function(opts) {
    this.message = `http:
});

because riot-compiler is supporting //not a comment inside single or double quatations, but not in backticks.
https://github.com/riot/compiler/blob/v3.1.1/lib/brackets.js#L23

@aMarCruz we could add a backtick support like below. Is it realistic? I'm not sure about its side effects...

// current
var R_STRINGS = /"[^"\\]*(?:\\[\S\s][^"\\]*)*"|'[^'\\]*(?:\\[\S\s][^'\\]*)*'/g
// with backticks
var R_STRINGS = /"[^"\\]*(?:\\[\S\s][^"\\]*)*"|'[^'\\]*(?:\\[\S\s][^'\\]*)*'|`[^`\\]*(?:\\[\S\s][^`\\]*)*`/g
@jpoppe

This comment has been minimized.

Show comment
Hide comment
@jpoppe

jpoppe Dec 20, 2016

@cognitom Thanks for your quick and extensive reply!

FYI: I am building a moderate application with Riot and use many of the ES6 features (actually about everywhere I could), I don't transpile anything (except my Webpack configuration so I could use imports there), neither I am using polyfills and everything runs great on the latest Chrome/Firefox releases. (could even use Async/Await (ES7) on the latest Chrome stable (and from March 2017 on Firefox stable).

This is the only issue I've encountered so far. (I was also surprised, but it seems ES6 is coming along nicely). That's the reason why I have filed it as a bug.

jpoppe commented Dec 20, 2016

@cognitom Thanks for your quick and extensive reply!

FYI: I am building a moderate application with Riot and use many of the ES6 features (actually about everywhere I could), I don't transpile anything (except my Webpack configuration so I could use imports there), neither I am using polyfills and everything runs great on the latest Chrome/Firefox releases. (could even use Async/Await (ES7) on the latest Chrome stable (and from March 2017 on Firefox stable).

This is the only issue I've encountered so far. (I was also surprised, but it seems ES6 is coming along nicely). That's the reason why I have filed it as a bug.

@aMarCruz

This comment has been minimized.

Show comment
Hide comment
@aMarCruz

aMarCruz Dec 29, 2016

Member

@cognitom , ES6 Template Literals, even in basic usage, are too complex for simple regexes.

Member

aMarCruz commented Dec 29, 2016

@cognitom , ES6 Template Literals, even in basic usage, are too complex for simple regexes.

@cognitom

This comment has been minimized.

Show comment
Hide comment
@cognitom

cognitom Dec 29, 2016

Member

@aMarCruz yeah, I worry about it, too. Do you think it's better to leave it as a known limitation?

Member

cognitom commented Dec 29, 2016

@aMarCruz yeah, I worry about it, too. Do you think it's better to leave it as a known limitation?

@aMarCruz

This comment has been minimized.

Show comment
Hide comment
@aMarCruz
Member

aMarCruz commented Dec 29, 2016

@cognitom yes.

@jpoppe

This comment has been minimized.

Show comment
Hide comment
@jpoppe

jpoppe Dec 29, 2016

@aMarCruz I use them everywhere in my Riot application and did not find/encounter any other issues or drawbacks using them, is there anything I could do to help?

jpoppe commented Dec 29, 2016

@aMarCruz I use them everywhere in my Riot application and did not find/encounter any other issues or drawbacks using them, is there anything I could do to help?

@josephrocca

This comment has been minimized.

Show comment
Hide comment
@josephrocca

josephrocca Jan 4, 2017

I'm a riot noob, and am wondering why regex is being used during the compilation process? Is it to extract the short-hand es6-like function declarations and scripts not enclosed with <script> tags? If so, could riot potentially have a "strict mode" where only real javascript can be used, javascript must be inside script tags, etc. Which would allow us to use back ticks? I think there's a fair chunk of the riotjs community that isn't too keen on the short-hand syntax anyway. I mostly agree with the reasons to avoid shorthand from this style guide, but I especially avoid it because of the (apparent) lack of support for async/await: #2195

josephrocca commented Jan 4, 2017

I'm a riot noob, and am wondering why regex is being used during the compilation process? Is it to extract the short-hand es6-like function declarations and scripts not enclosed with <script> tags? If so, could riot potentially have a "strict mode" where only real javascript can be used, javascript must be inside script tags, etc. Which would allow us to use back ticks? I think there's a fair chunk of the riotjs community that isn't too keen on the short-hand syntax anyway. I mostly agree with the reasons to avoid shorthand from this style guide, but I especially avoid it because of the (apparent) lack of support for async/await: #2195

@aMarCruz

This comment has been minimized.

Show comment
Hide comment
@aMarCruz

aMarCruz Jan 5, 2017

Member

@josephrocca, riot has no real parser, regexes are used to extract the htnl, css, js and expressions. Since ES6 Template Literals are multiline, can be nested an contain almost any character, we cannot guarantee the correct extraction of the parts.

A real, specialized parser with ES6 TL support is in our plans.

Member

aMarCruz commented Jan 5, 2017

@josephrocca, riot has no real parser, regexes are used to extract the htnl, css, js and expressions. Since ES6 Template Literals are multiline, can be nested an contain almost any character, we cannot guarantee the correct extraction of the parts.

A real, specialized parser with ES6 TL support is in our plans.

@josephrocca

This comment has been minimized.

Show comment
Hide comment
@josephrocca

josephrocca Jan 5, 2017

@aMarCruz Thanks for the reply. Given the issues it causes (also see 2198 linked above), this should probably be in the documentation somewhere? (I missed it if it is already in there) It sounds like it has caused a few people some debugging headaches

josephrocca commented Jan 5, 2017

@aMarCruz Thanks for the reply. Given the issues it causes (also see 2198 linked above), this should probably be in the documentation somewhere? (I missed it if it is already in there) It sounds like it has caused a few people some debugging headaches

@GianlucaGuarini

This comment has been minimized.

Show comment
Hide comment
@GianlucaGuarini

GianlucaGuarini Jan 9, 2017

Member

@cognitom could you make a pull request to riot-compiler? @aMarCruz I think we can easily handle also the template literal like the other js features let's give it a try

Member

GianlucaGuarini commented Jan 9, 2017

@cognitom could you make a pull request to riot-compiler? @aMarCruz I think we can easily handle also the template literal like the other js features let's give it a try

@aMarCruz

This comment has been minimized.

Show comment
Hide comment
@aMarCruz

aMarCruz Jan 9, 2017

Member

@GianlucaGuarini sure, it can be done if we limit the support to very basic ES6 TL (i.e. not nested, not complex expressions inside brackets, no tag-like contructions, etc) and escaping single/double quotes inside these.

Member

aMarCruz commented Jan 9, 2017

@GianlucaGuarini sure, it can be done if we limit the support to very basic ES6 TL (i.e. not nested, not complex expressions inside brackets, no tag-like contructions, etc) and escaping single/double quotes inside these.

@GianlucaGuarini

This comment has been minimized.

Show comment
Hide comment
@GianlucaGuarini

GianlucaGuarini Jan 10, 2017

Member

@aMarCruz of course we can just provide a basic support for them, at some point our users should decide whether to use a transpiler to handle more complicate transformations.

Member

GianlucaGuarini commented Jan 10, 2017

@aMarCruz of course we can just provide a basic support for them, at some point our users should decide whether to use a transpiler to handle more complicate transformations.

GianlucaGuarini added a commit to riot/compiler that referenced this issue Feb 19, 2017

@jpoppe

This comment has been minimized.

Show comment
Hide comment
@jpoppe

jpoppe Feb 19, 2017

GianlucaGuarini, thanks again!!!! Was waiting for this :) 👍

jpoppe commented Feb 19, 2017

GianlucaGuarini, thanks again!!!! Was waiting for this :) 👍

@GianlucaGuarini

This comment has been minimized.

Show comment
Hide comment
@GianlucaGuarini

GianlucaGuarini Feb 19, 2017

Member

@jpoppe I guess this issue is not completely solved yet

Member

GianlucaGuarini commented Feb 19, 2017

@jpoppe I guess this issue is not completely solved yet

@GianlucaGuarini

This comment has been minimized.

Show comment
Hide comment
@GianlucaGuarini

GianlucaGuarini Feb 19, 2017

Member

Ok something went wrong publishing the new compiler I will solve the issue asap

Member

GianlucaGuarini commented Feb 19, 2017

Ok something went wrong publishing the new compiler I will solve the issue asap

@kazzkiq

This comment has been minimized.

Show comment
Hide comment
@kazzkiq

kazzkiq Feb 20, 2017

Contributor

Just stumbled on this issue in a project. It seems that template literals work fine, but after they're used, any line containing double slash comments end up in a parse error.

ex.:

addItem(e) {
  let item = `Item ${e.item.number}`;
}

removeItem() {
  // this removes an item
  this.function.will.not.work.because.of.comment.above();
}

In the above case, removeItem() isn't parsed as a tag function and simply fires an error in the browser. The rest of the code (before the slash comment) is parsed fine.

Regex also fire this behavior (e.g. /\w+/g). It seems anywhere containing two or more slashes after a template literal trigger the issue.

Contributor

kazzkiq commented Feb 20, 2017

Just stumbled on this issue in a project. It seems that template literals work fine, but after they're used, any line containing double slash comments end up in a parse error.

ex.:

addItem(e) {
  let item = `Item ${e.item.number}`;
}

removeItem() {
  // this removes an item
  this.function.will.not.work.because.of.comment.above();
}

In the above case, removeItem() isn't parsed as a tag function and simply fires an error in the browser. The rest of the code (before the slash comment) is parsed fine.

Regex also fire this behavior (e.g. /\w+/g). It seems anywhere containing two or more slashes after a template literal trigger the issue.

GianlucaGuarini added a commit to riot/compiler that referenced this issue Mar 18, 2017

Merge branch 'master' into next
* master:
  updated: dev dependencies
  3.2.1
  closes riot/riot#2167
  3.2.0

GianlucaGuarini added a commit to riot/compiler that referenced this issue May 20, 2017

Merge branch 'master' into dev
* master:
  updated: dev dependencies
  3.2.1
  closes riot/riot#2167
  3.2.0
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment