Skip to content
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

Template syntax for each breaks with reserved words as single-item variables #934

Closed
stffrd opened this issue Nov 21, 2017 · 8 comments
Closed

Comments

@stffrd
Copy link

stffrd commented Nov 21, 2017

I ran into an issue where an {{#each}} in my templating code wasn't outputting values I was passing through an array.

My code:

<div>
    {{#each cases as case}}
    <h2>{{case.title}}</h2>
    {{/each}}
</div>

The shape of my data:

export default [
    {   
        title   : "One",
        options : {},
        data    : {
            
        }
    },

    {
        title   : "Two",
        options : {},
        data    : {
            
        }
    },

    {
        title   : "Three",
        options : {},
        data    : {
            
        }
    }
];

Looks like since case is a reserved word in javascript, I can't use it in my templating. 😢
Same goes for pretty much any other reserved word, You can remove the last letter in any of the REPL examples below and see it actually render properly.

REPL repro with "case"
REPL repro with "class"
REPL repro with "default"

@stffrd stffrd changed the title Template syntax for Each breaks with js reserved words as variables Template syntax for each breaks with reserved words as single-item variables Nov 21, 2017
@Rich-Harris
Copy link
Member

This is a tricky, err... case. The problem is that acorn (which parses JS on Svelte's behalf) won't parse case.video, because it's not actually valid JavaScript.

It's not something particular to Svelte — this would be equally impossible:

cases.forEach(case => {
  console.log(case.title);
});

We might just have to accept that it isn't possible to have reserved words in template expressions, sadly! Unless anyone has any bright ideas?

@aubergene
Copy link

Seems fine as is, a regular JS annoyance. Maybe if you wanted to be extra nice then in dev mode it could give a warning if any template identifier is a reversed keyword?

@tivac
Copy link
Contributor

tivac commented Nov 22, 2017

I must have a fundamental misunderstanding of how svelte compiles templates, because I assumed that it would be trivial for it to rename those values inside of a template. When using template tags like svelte does I tend not to think of it as "just javascript" any more, so I definitely fell into the same trap that @Morklympious did and assumed that would be fine at first.

@Rich-Harris
Copy link
Member

I assumed that it would be trivial for it to rename those values inside of a template

You can't just do a string replace (because you might have something like {{es6 ? "let : "var"}}) — you have to have an AST that you can traverse. But in order to generate an AST, it has to be valid JavaScript. (Unless you implement your own parser that allows illegal identifiers, but that seems... excessive 😀.)

I think @aubergene is onto the right idea — Svelte should just throw an error at compile-time that says 'you can't use case as an each-block context because it's a reserved word', or something that makes the issue explicit. Unfortunately that wouldn't handle the situation where case is a property of your data object. But it's better than nothing.

@tivac
Copy link
Contributor

tivac commented Nov 22, 2017

Yeah, I get why acorn doesn't like that name. I assumed that svelte used a custom parser for the special bits of template syntax (since {{#each ...}} and {{#if ... }} are clearly not valid JS) that would allow for those sorts of rewrites if invalid identifiers were used.

It's not a big deal, ultimately, and I agree that even just slightly tweaked error messaging would certainly solve most of the problem.

@tivac
Copy link
Contributor

tivac commented Nov 22, 2017

After some more thinking I get the issue, I wasn't thinking about later uses of {{case}} that are parsed by acorn and then explode. The {{#each ... }} isn't actually the problem.

Sorry for the flopping around, had to learn some more svelte before the truth dawned on me.

@stffrd
Copy link
Author

stffrd commented Nov 22, 2017

I think @aubergene's "Throw an error" is an acceptable solution, if only to stop people from falling down the rabbit hole of "code, what are you DOING?!".

Rich-Harris added a commit that referenced this issue Nov 23, 2017
throw error on illegal context
@Rich-Harris
Copy link
Member

This is implemented in 1.43:

screen shot 2017-11-25 at 10 09 57 pm

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants