[[FIX]] es6: export on var, let, const exports all variables, not just the first one #2252
Conversation
|
oh man, I thought this was filed on traceur for some reason. Mark this as fixing the issue I just opened then, hah =) |
| @@ -863,7 +863,7 @@ var JSHINT = (function() { | |||
| funct["(blockscope)"].stack(); | |||
| advance("let"); | |||
| advance("("); | |||
| state.syntax["let"].fud.call(state.syntax["let"].fud, false); | |||
| state.syntax["let"].fud.call(state.syntax["let"].fud); | |||
There was a problem hiding this comment.
@rwaldron do you have any idea why we're calling it this way instead of state.tokens.curr.fud(false);? or not "curr" in this case but "prev"
There was a problem hiding this comment.
seems like it was a common pattern - found lots of commits using it - 5924b2a#diff-180beb5359ee54413144b3d8200d7504R3975
one thing is I didn't even notice it because its hard to compare & notice that state.syntax["let"].fud is repeated identically.
removing it doesn't have any effect, further notice that commit has a mix of different styles. I can't find a single committer introducing one style or the other.
Most of the other non-let/var/const don't use call. My vote would be to remove the call. I have an extra commit waiting, just say the word.
There was a problem hiding this comment.
The problem is if any of these constructs ever mutate their this binding, then you end up with problems if calling them that way, so I would prefer we didn't do that
There was a problem hiding this comment.
that, and it's just harder to read the .call() invocations =)
There was a problem hiding this comment.
you want me to push the change onto this PR?
| if (inexport) { | ||
| exported[state.tokens.next.value] = true; | ||
| state.tokens.next.exported = true; | ||
| } |
There was a problem hiding this comment.
setting the exported flag should happen after the call to identifier()(on the next line, which will make sure that it's actually an identifier)
|
other than those comments it looks good to me |
|
Thanks, I understand now - was my first time looking at the code, so I didn't realise you could get curr and prev and that was the same as lookup on statements. |
| "export class MyClass { }", | ||
| "export var varone = 1, vartwo = 2;", | ||
| "export const constone = 1, consttwo = 2;", | ||
| "export let letone = 1, lettwo = 2;" |
|
lgtm, thanks |
Closes jshint#2248 Closes jshint#2253 Closes jshint#2252
Fixes #2248
Looks like code will have to be expanded again to work with export and de-structuring in the same var/let/const but I think its alot more work. This looks to me like a move in the right direction and fixes the bug and add tests.
I've signed the CLA.