Fixed computed keys for class expression #10029
Conversation
tanhauhau
force-pushed
the
tanhauhau/fix/computed-keys
branch
from
e9cdda3
to
e20517e
Compare
tanhauhau
force-pushed
the
tanhauhau/fix/computed-keys
branch
from
e20517e
to
aaf48aa
Compare
|
Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/10878/ |
| @@ -85,6 +85,9 @@ export function injectInitialization(path, constructor, nodes, renamer) { | |||
| export function extractComputedKeys(ref, path, computedPaths, file) { | |||
| const declarations = []; | |||
|
|
|||
| const nearestBlock = path.find( | |||
| parentPath => parentPath.isBlockStatement() || parentPath.isProgram(), | |||
There was a problem hiding this comment.
Sometimes the variable needs to be inserted where a block doesn't exist. Do we have a test for this code?
() => class { [k] = 2 }There was a problem hiding this comment.
good catch, added a new test case here (https://github.com/babel/babel/pull/10029/files#diff-fe07c1ba4a5c933d73ca27c26a08737a)
nicolo-ribaudo
added
PR: Bug Fix 🐛
| @@ -98,10 +98,14 @@ export function extractComputedKeys(ref, path, computedPaths, file) { | |||
| const ident = path.scope.generateUidIdentifierBasedOnNode( | |||
| computedNode.key, | |||
| ); | |||
| path.scope.push({ | |||
| id: ident, | |||
| kind: "let", | |||
There was a problem hiding this comment.
I don't see any advantage in using let here? Usually we try to generate an output as low-level as possible, if it isn't harder to do.
There was a problem hiding this comment.
It is for this scenario: microsoft/TypeScript#27864
if it is set as var, it will behave the same as pointed out in the issue link above, which will break this newly added test case (https://github.com/babel/babel/pull/10029/files#diff-fbbdd83e7a9c998721c1484529c2ce92)
maybe it should be determined by whether the computedKeys original declaration kind, to preserve the original scoping intention?
There was a problem hiding this comment.
Oh ok; it's ok like this then.
There was a problem hiding this comment.
Seems like adding a comment explaining this might be useful?
|
Thank you! |
lock
bot
added
the
outdated
letinstead ofvarfor declaring computedKeys variable, so that the value is correctly scoped.letat the top of current block, so that it wont get bailed out when converting the declaration to expression sequence.