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

Only import types from declared dependencies #16494

Merged
merged 13 commits into from
May 30, 2024

Conversation

nicolo-ribaudo
Copy link
Member

@nicolo-ribaudo nicolo-ribaudo commented May 15, 2024

Q                       A
Fixed Issues? Fixes #1, Fixes #2
Patch: Bug Fix?
Major: Breaking Change?
Minor: New Feature?
Tests Added + Pass? Yes
Documentation PR Link
Any Dependency Changes?
License MIT

This PR makes sure that all our import types only reference packages that are listed in dependencies/peerDependencies. This is done mainly in two ways:

  1. plugins all peerDepend on @babel/core, so they can import NodePath/Visitor/Scope/types from there rather than from @babel/traverse/@babel/types
  2. helpers can depend on @babel/traverse/@babel/types: if a helper expects a NodePath as a parameter, you already have to have @babel/traverse/@babel/types in your dependencies so this is not causing extra deps.

To (2) there are some exceptions:

  • babel-helper-builder-react-jsx and babel-helper-plugin-utils only make sense when using with a full plugin (and not directly with parser+traverse), so I added @babel/core as a peer dependency (but only for Babel 8)
  • babel-helper-fixtures is also only needed when testing plugins, so I added @babel/core as a peer dep

This PR doesn't lint @babel/standalone and @babel/parser since those are bundled, but we should probably add @babel/types as a dependency of @babel/parser. I don't really like doing it since it's perfectly reasonable to use @babel/parser without also using @babel/types, but on the other hand if the dependency is only used in import type it won't actually affect anything at runtime (it won't be loaded) so adding it is not too bad.

There is one remaining lint error fixed by #16493.

@babel-bot
Copy link
Collaborator

babel-bot commented May 15, 2024

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

@nicolo-ribaudo nicolo-ribaudo marked this pull request as draft May 15, 2024 16:57
@@ -1,3 +1,4 @@
// eslint-disable-next-line import/no-extraneous-dependencies -- TODO: Avoid cycle
Copy link
Member Author

Choose a reason for hiding this comment

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

There are 4 packages whose types circularly depend on @babel/traverse. Lets just ignore those errors for now (@babel/traverse will be in the dependencies anyway), but I will think of a better solution before going stable.

@nicolo-ribaudo nicolo-ribaudo marked this pull request as ready for review May 22, 2024 14:45
@nicolo-ribaudo nicolo-ribaudo added the PR: Internal (next major) 🏠 A type of pull request used for our changelog categories for next major release label May 22, 2024
@nicolo-ribaudo nicolo-ribaudo merged commit 90fdd7e into babel:main May 30, 2024
51 checks passed
@nicolo-ribaudo nicolo-ribaudo deleted the no-missing-deps branch May 30, 2024 07:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: Internal (next major) 🏠 A type of pull request used for our changelog categories for next major release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants