-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
[BUGFIX release] Fix overwriting rest positional parameters when passed as named parameters. #14509
Conversation
There is a test failing because jshint does not like the line:
And cannot find a way around it :$ I guess this needs to be backported to LTS since it is a regression introduced partially in 2.7 (no transition) and 2.8 (breaks). Checking this next. Thank you for taking the time to look at this PR! |
I checked and this commit can be cherry-picked on top of lts-2-8 without problem. I have also a branch with that done in my fork. |
Uhm, the test is not failing in CI but in local does :$ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes here look good, can you PR the new tests against master so we keep them?
Also, does this need to be pulled back into lts-2-8 (now that 2.9 is release
)? If it does, can you update commit message to be [BUGFIX lts-2-8]
?
@@ -10,6 +10,7 @@ QUnit.module('Ember Metal Utils'); | |||
QUnit.test('inspect outputs the toString() representation of Symbols', function() { | |||
// Symbol is not defined on pre-ES2015 runtimes, so this let's us safely test | |||
// for it's existence (where a simple `if (Symbol)` would ReferenceError) | |||
/* jshint latedef: false */ | |||
let Symbol = Symbol || null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is pretty wild. Can we just change it (future PR is fine)?
I think this is a better guard (below):
if (typeof Symbol !== 'undefined') {
}
Then we don't have to do any global clobbering variables...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is surprising, why wouldn't let S = S || null
throw a ReferenceError
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the following very much fails:
(function() { let Symbol = Symbol || null;}());
with
VM257:1 Uncaught ReferenceError: Symbol is not defined
let Symbol
ensures that before the let
that Symbol
is in a TDZ. basically, it is non-addressable until after the let
. As it turns out, the right hand side of let Symbol = <right hand side>
is technically before the let
, and since let
has reserved Symbol
for that scope, even if the a global Symbol
exists, this should error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Babel transpile apparently does not handle the let
TDZ correctly. https://babeljs.io/repl/#?babili=false&evaluate=true&lineWrap=false&presets=es2015%2Creact%2Cstage-2&code=(function()%20%7B%20let%20Symbol%20%3D%20Symbol%20%7C%7C%20null%3B%7D())%3B%0A
It should really be an error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
opened a comment on a closed babel issue: babel/babel#527 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
left a comment (can add a tdz option to the plugin)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hzoo thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
☔ The latest upstream changes (presumably #14531) made this pull request unmergeable. Please resolve the merge conflicts. |
@@ -10,6 +10,7 @@ QUnit.module('Ember Metal Utils'); | |||
QUnit.test('inspect outputs the toString() representation of Symbols', function() { | |||
// Symbol is not defined on pre-ES2015 runtimes, so this let's us safely test | |||
// for it's existence (where a simple `if (Symbol)` would ReferenceError) | |||
/* jshint latedef: false */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you remove this and rebase now that #14531 is landed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On it!
…ed as named parameters. If rest positional parameters are passed as named arguments, it might not be passed to the component when called with a contextual component. Example: ```hbs {{component (component "link-to") params=someParams}} ``` Fix emberjs#14508
See emberjs#14508 and emberjs#14509 for more details on the bug.
Pulled into lts-2-8 branch |
If rest positional parameters are passed as named arguments, it might
not be passed to the component when called with a contextual component.
Example:
Fix #14508