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

Cleanup comment removal + documentation + release #26

Merged
merged 1 commit into from
May 30, 2024

Conversation

AndrewRayCode
Copy link
Collaborator

@AndrewRayCode AndrewRayCode commented May 30, 2024

Spelling, comments I don't care about anymore, forgot to add error.d.ts apparently, it was getting published, but not in source control!

@davepagurek
Copy link
Contributor

Sounds good, I can also understand the behaviour being potentially confusing, so following Babel's lead makes sense. let me know what you find!

@AndrewRayCode
Copy link
Collaborator Author

It looks like babel does continue to visit the nodes you replace. Here's an example replacing a function call - and it has infinite recursion because it looks like it keeps visitng the replaced node https://codesandbox.io/p/sandbox/ast-forked-r9fpyx?file=%2Fsrc%2Findex.js%3A25%2C33 (warning this codesandbox triggers a maximum call stack)

I think there's another parity change to make for babel, which is the replacer node itself should also get visited, not just its children

@davepagurek
Copy link
Contributor

Ahh ok right, want me to make a PR adjusting my changes from earlier to just traverse the replaced node directly if it exists?

@AndrewRayCode
Copy link
Collaborator Author

AndrewRayCode commented May 30, 2024

@davepagurek if you are up for it that would be great! I was going to look at it this weekend, if you get to it before then that would be awesome. I'll merge rest of the clean-up changes in this PR.

I think there should be a test for the visit behavior explicitly. In the test, do something like visit an identifier, replace it, and verify the replaced node is also visited and that the original node is not visited.

Spelling, comments I don't care about anymore, forgot to add error.d.ts
apparenty, it was getting published, but not in source control!
@AndrewRayCode AndrewRayCode changed the title Proposed change to not visit replaced children Cleanup comment removal + documentation + release May 30, 2024
@AndrewRayCode AndrewRayCode merged commit f042dc0 into main May 30, 2024
1 check passed
@AndrewRayCode AndrewRayCode deleted the dont-visit-replaced-children branch May 30, 2024 22:09
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.

2 participants