Skip to content

add 12 missing NODE_FIELDS#13577

Merged
JLHwung merged 6 commits intobabel:mainfrom
jedwards1211:fix/babel-types-fields
Jul 23, 2021
Merged

add 12 missing NODE_FIELDS#13577
JLHwung merged 6 commits intobabel:mainfrom
jedwards1211:fix/babel-types-fields

Conversation

@jedwards1211
Copy link
Copy Markdown
Contributor

@jedwards1211 jedwards1211 commented Jul 20, 2021 •
edited
Loading

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
Copy Markdown

codesandbox-ci Bot commented Jul 20, 2021 •
edited
Loading

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
Copy Markdown
Collaborator

babel-bot commented Jul 20, 2021 •
edited
Loading

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

JLHwung
JLHwung reviewed Jul 20, 2021
View reviewed changes
Comment thread packages/babel-types/package.json Outdated
"@babel/parser": "workspace:*",
"chalk": "^4.1.0"
"chalk": "^4.1.0",
"fs-extra": "^10.0.0",
Copy link
Copy Markdown
Contributor

@JLHwung JLHwung Jul 20, 2021

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
Copy Markdown
Contributor Author

@jedwards1211 jedwards1211 Jul 20, 2021

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
Copy Markdown
Contributor Author

@jedwards1211 jedwards1211 Jul 20, 2021 •
edited
Loading

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
Copy Markdown
Contributor Author

jedwards1211 commented Jul 20, 2021

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
Copy Markdown
Member

nicolo-ribaudo commented Jul 20, 2021 •
edited
Loading

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

@nicolo-ribaudo
Copy link
Copy Markdown
Member

nicolo-ribaudo commented Jul 20, 2021 •
edited
Loading

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
Copy Markdown
Contributor Author

jedwards1211 commented Jul 20, 2021 •
edited
Loading

@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
Copy Markdown
Member

nicolo-ribaudo commented Jul 20, 2021

@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
Copy Markdown
Contributor Author

jedwards1211 commented Jul 22, 2021 •
edited
Loading

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
Copy Markdown
Member

nicolo-ribaudo commented Jul 22, 2021

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
@jedwards1211 @nicolo-ribaudo
add 12 missing NODE_FIELDS (babel#13577)
c97f9c4 
* 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
@sync-by-unito sync-by-unito Bot mentioned this pull request Aug 2, 2021
Closed
@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.

Reviewers

@JLHwung JLHwung JLHwung approved these changes

@nicolo-ribaudo nicolo-ribaudo nicolo-ribaudo approved these changes

Assignees

No one assigned

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

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[Bug]: readonly, declare, and definite are missing from NODE_FIELDS.ClassPrivateProperty [Bug]: variance is missing from NODE_FIELDS.ClassProperty

4 participants

@jedwards1211 @babel-bot @nicolo-ribaudo @JLHwung