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

Ensure no duplicated AST nodes are produced #58

Merged
merged 1 commit into from Sep 1, 2019

Conversation

kjagiello
Copy link
Contributor

Our babel plugin produced an invalid AST which caused some inconsistencies in some projects, where babel would generate invalid JS code. The reason for that was that we have reused some AST nodes without cloning them. More background here.

Solves #57.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 96.445% when pulling 11680db on bugfix/no-duplicated-ast-nodes into b0a34c9 on master.

Copy link
Contributor

@lydell lydell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome! I learned a lot from this PR.

It’s weird that we’ve used djedi-react for over a year and haven’t run into any issues until now!

@@ -66,6 +75,18 @@ _djedi.reportPrefetchableNode({
`);
});

test("ensure no duplicated AST nodes are found", () => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! Didn’t know about babel-check-duplicated-nodes before.

@kjagiello
Copy link
Contributor Author

Yaeh, super weird! A friend of mine used this library in her project and stumbled upon this issue when trying to get SSR to work. Although SSR has never triggered this issue in our projects, I was able to reproduce it by running babel-cli.

Merging this in then! Thanks for taking a look at this!

@kjagiello kjagiello merged commit 025621c into master Sep 1, 2019
@kjagiello kjagiello deleted the bugfix/no-duplicated-ast-nodes branch September 1, 2019 20:32
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

Successfully merging this pull request may close these issues.

None yet

3 participants