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

add 12 missing NODE_FIELDS #13577

Merged
merged 6 commits into from Jul 23, 2021
Merged

Conversation

jedwards1211
Copy link
Contributor

@jedwards1211 jedwards1211 commented Jul 20, 2021

fix #13558
fix #13563

QUESTION: Can ExportDefaultDeclaration.exportKind be anything other than "value"? I looked through the parser code and tried to come up with Flow or TS example that would result in exportKind: "type", but couldn't find any.

Q                       A
Fixed Issues? Fixes #1, Fixes #2
Patch: Bug Fix? Yes
Major: Breaking Change? No
Minor: New Feature? No
Tests Added + Pass? Yes
Documentation PR Link
Any Dependency Changes? Added glob and fs-extra to devDependencies of @babel/types
License MIT

@jedwards1211 jedwards1211 changed the title add missing NODE_FIELDS add 25 missing NODE_FIELDS Jul 20, 2021
@jedwards1211 jedwards1211 changed the title add 25 missing NODE_FIELDS add 12 missing NODE_FIELDS Jul 20, 2021
@codesandbox-ci
Copy link

codesandbox-ci bot commented Jul 20, 2021

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 60f3126:

Sandbox Source
babel-repl-custom-plugin Configuration
babel-plugin-multi-config Configuration

@babel-bot
Copy link
Collaborator

babel-bot commented Jul 20, 2021

Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/47487/

@@ -30,7 +30,9 @@
"devDependencies": {
"@babel/generator": "workspace:*",
"@babel/parser": "workspace:*",
"chalk": "^4.1.0"
"chalk": "^4.1.0",
"fs-extra": "^10.0.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess we can replace fs-extra by JSON.parse(fs.readFileSync(#))?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just switched it to promisify(fs.readFile), wouldn't want to block the entire process

Copy link
Contributor Author

Choose a reason for hiding this comment

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

argh, didn't realize tests run on node 8 though,I'll promisify it manually

@jedwards1211
Copy link
Contributor Author

There are lint errors I didn't cause:

Error: packages/babel-traverse/src/path/lib/virtual-types.ts(119,7): error TS2578: Unused '@ts-expect-error' directive.
Error: packages/babel-generator/src/generators/flow.ts(550,3): error TS2578: Unused '@ts-expect-error' directive.
Error: packages/babel-generator/src/generators/flow.ts(555,5): error TS2578: Unused '@ts-expect-error' directive.

@nicolo-ribaudo
Copy link
Member

nicolo-ribaudo commented Jul 20, 2021

Whops thanks, I'll fix them on main. I'm surprised CI is green there.

@nicolo-ribaudo
Copy link
Member

nicolo-ribaudo commented Jul 20, 2021

Can ExportDefaultDeclaration.exportKind be anything other than "value"?

No, but we still set it for consistency with the other export and import declarations.

@jedwards1211
Copy link
Contributor Author

jedwards1211 commented Jul 20, 2021

@nicolo-ribaudo okay, well it sets exportKind: "value" of ExportDefaultDeclarations when parsing TS, but not when parsing Flow, so I think that's a bug with parsing Flow then

@nicolo-ribaudo
Copy link
Member

@jedwards1211 The linting failures seem to be caused but this PR, but it's a good thing since we were just telling TS "we know that our code doesn't respect the declared type, so there is an error here".

You can just delete those three @ts-expect-error comments.

@jedwards1211
Copy link
Contributor Author

jedwards1211 commented Jul 22, 2021

Oh now I get it, someone put those @ts-expect-errors there to suppress errors due to the missing fields...but those were valid errors...
Hopefully the fields weren't intentionally missing for some reason?

@nicolo-ribaudo
Copy link
Member

It's just that we use Flow and TS nodes in the plugins way less than JS nodes, so we never noticed it. Thanks for this PR!

@nicolo-ribaudo nicolo-ribaudo added pkg: types PR: Bug Fix 🐛 A type of pull request used for our changelog categories labels Jul 22, 2021
@JLHwung JLHwung merged commit 1d48bb0 into babel:main Jul 23, 2021
nicolo-ribaudo pushed a commit to nicolo-ribaudo/babel that referenced this pull request Jul 30, 2021
* test: add fields test

* fix(babel-types): add missing NODE_FIELDS and tests

fix babel#13558
fix babel#13563

* chore: avoid using fs-extra

* chore: code cleanup

* chore: avoid util.promisify

* fix: remove bad ts-expect-error suppressions
@github-actions github-actions bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Oct 23, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 23, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
outdated A closed issue/PR that is archived due to age. Recommended to make a new issue pkg: types PR: Bug Fix 🐛 A type of pull request used for our changelog categories
Projects
None yet
4 participants