Skip to content

Add a missing dependency on @babel/types to @babel/parser#15042

Closed
Andarist wants to merge 1 commit intomainfrom
types/parser-missing-dep
Closed

Add a missing dependency on @babel/types to @babel/parser#15042
Andarist wants to merge 1 commit intomainfrom
types/parser-missing-dep

Conversation

@Andarist
Copy link
Member

@Andarist Andarist commented Oct 14, 2022

This is an actual dependency of the typings contained in this package and thus should be declared explicitly:

): ParseResult<import("@babel/types").File>;

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

@Andarist Andarist force-pushed the types/parser-missing-dep branch from aa5ce21 to 571bb7c Compare October 14, 2022 14:21
@babel-bot
Copy link
Collaborator

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

@JLHwung
Copy link
Contributor

JLHwung commented Oct 14, 2022

cf. #10315, #9924

@Andarist
Copy link
Member Author

I see the concern with the added dependency - but without this, it just doesn't work with strict package managers and can cause some weird issues in the ones that rely on the flattened node_modules. I had an issue today where different packages were accidentally loading @babel/types from different locations and thus causing type issues because those types were not quite compatible. This wouldn't happen if the dependency would be declared explicitly.

@liuxingbaoyu
Copy link
Member

I personally prefer to remove the import and return any or {type:xxx...}.

@merceyz
Copy link
Contributor

merceyz commented Oct 15, 2022

but without this, it just doesn't work with strict package managers

While that is correct both Yarn and pnpm declare this dependency automatically so in this case they're not affected.
https://github.com/yarnpkg/berry/blob/444eea50e8bdbd8c6a3afef51633a011e4f30069/packages/yarnpkg-extensions/sources/index.ts#L160-L165

@liuxingbaoyu
Copy link
Member

liuxingbaoyu commented Oct 29, 2022

I've looked at the past two PRs and it seems that only bundling of types is an acceptable solution.
Currently I'm doing something similar in #15094, if we want the types of @babel/types to be bundled together as well, then I don't have to try to suppress the error warning from ts . (see Failed CI)

Thanks to merceyz for the information, in my opinion, since yarn and pnpm already have @babel/types as a dependency, it is acceptable for us to add it.
But maybe packaging the types can report some compatibility issues for users, I'm not sure about that.

@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 30, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 30, 2024
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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants