Fix class properties tdz decorator interop #7475
Conversation
maurobringolf
changed the title
|
Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/7088/ |
| @@ -0,0 +1,7 @@ | |||
| function dec() {} | |||
There was a problem hiding this comment.
could we test something here? it seems that this show only the output, so technically could be just a fixture test, u dont actually evaluate the output right now so i think it doesnt justify using exec.js
There was a problem hiding this comment.
Before the fix exec.js incorrectly threw a runtime error. There was no way to determine the correct output.js fixture back then. Personally, I would add the (currently defunct) proposal-decoratoras transform to make sure that once #6107 lands, independently of how both transforms interplay, they still yield valid, executable code.
There was a problem hiding this comment.
Converting to a fixture makes sense to me. I renamed to input.js but was unable to get an output generated locally. Committed to see what the CI does.
Also I noticed that some other tests are called input.js but executed? For example https://github.com/babel/babel/tree/master/packages/babel-plugin-proposal-class-properties/test/fixtures/static-property-tdz-edgest-case.
EDIT: figured out how to organize test files correctly. I generated an output.js and converted to fixture test.
|
Can we remove the yarn.lock? |
|
@existentialism Sure, I removed it |
danez
added
Spec: Decorators
PR: Bug Fix 🐛
|
Do you need anything from me for this one? I'd really like to get this in, since there are a lot of potential bugs with renamed classes with the current code. |
|
Is there a plan for this? |
|
Yeah sorry, currently bugs related to the old decorators specification aren't getting much attention. We are working on implementing the new decorators proposal (#7976) which shouldn't be affected by many of these bugs. |
|
Thats understandable. Thanks for the reply. |
|
@nicolo-ribaudo but the issue is still present with Update: I'm responding to myself: it's a new name, but under the hood it still use the legacy decorators until the new one are available. |
| @@ -33,7 +33,7 @@ export default function(api, options) { | |||
| }, | |||
|
|
|||
| ReferencedIdentifier(path) { | |||
| if (this.classRef === path.scope.getBinding(path.node.name)) { | |||
| if (this.classBinding === path.scope.getBinding(path.node.name)) { | |||
There was a problem hiding this comment.
Instead of relying on false as a special value to indicate that the class doen't have an id, I think that we should check this.classBinding && this.classBinding === ... here, so that it is more obvious that it is not always defined.
|
Merged in #8463. Thank you! |
lock
bot
added
the
outdated
The decorators plugin does some renaming and rearranging of classes which can break my code from #6855 . I figured out that I am unnecessarily recomputing the class binding via
ref.namewhen I can get it directly viapath.scope.getBinding(path.node.id.name). I leftrefuntouched and renamed the class binding used in the TDZ check toclassBinding.I am not sure what's the best Git way of doing this. I added @buschtoens commit with the test case to my fork, so this PR contains his too.