-
Notifications
You must be signed in to change notification settings - Fork 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
polish(ast): prefer undefined over empty arrays #4206
Conversation
✅ Deploy Preview for compassionate-pike-271cb3 ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Hi @yaacovCR, I'm @github-actions bot happy to help you with this PR 👋 Supported commandsPlease post this commands in separate comments and only one per comment:
|
60e7d7f
to
aee3bb9
Compare
Well, it works! It would seem that it's more accurate to use undefined when an Peeking around at https://astexplorer.net/ it seems that TS parser has some empty arrays, some undefined, while for Babel I didn't on a very cursory look see any undefined. But still not 100% sure this is too compelling, with the status quo being somewhat compelling! Comments welcome! |
Merging this PR allows us to close #2203, improving robustness of our test coverage. Presumably our downstream users were also affected by #2203 and had to account for manually created AST nodes, so they should be robust to this change, but I upgraded it to a breaking change anyway just to call it out. |
Probably needs a rebase |
f5bf1bb
to
9e6f027
Compare
bdde1db
to
125c2b7
Compare
see: #2405 (comment)
Manually created ASTs always allowed undefined in place of empty arrays; this change simply updates the parser to more closely follow the manual approach. This is therefore not technically a breaking change, but considering that there may be tools not aware of this, we have labelled it a BREAKING_CHANGE to highlight it for consumers.
This closes #2203 as our tests now cover the undefined case by default whenever there is an empty array.