Skip to content

fix: ensure @babel/parser can reference @babel/types#10315

Closed
eventualbuddha wants to merge 1 commit intobabel:masterfrom
eventualbuddha:parser-depends-on-types
Closed

fix: ensure @babel/parser can reference @babel/types#10315
eventualbuddha wants to merge 1 commit intobabel:masterfrom
eventualbuddha:parser-depends-on-types

Conversation

@eventualbuddha
Copy link
Member

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

The type declarations for @babel/parser reference @babel/types but previously there was no explicit dependency on @babel/parser. This meant that TypeScript is unable to process @babel/parser unless @babel/types also happens to be installed. Furthermore, if installing using a stricter node_modules layout (like the one favored by pnpm), even if @babel/types is installed by another package @babel/parser will be unable to see it. With an explicit dependency this problem goes away.

See discussion on Slack.

@JLHwung
Copy link
Contributor

JLHwung commented Aug 11, 2019

@eventualbuddha Could you please rebase on master?

@nicolo-ribaudo
Copy link
Member

There is also #9924, where @loganfsmyth wrote a big valid concern.

@eventualbuddha
Copy link
Member Author

I agree with @loganfsmyth that it's a little strange to include a package as a dependency simply because you need the types. However, I don't see a good way around it. As-is, @babel/parser is broken when used by itself with respect to TypeScript.

@eventualbuddha
Copy link
Member Author

I’m unsure why this is failing. Looks like a git issue?

error Couldn't find match for "89eab830be040fbaabc31ad7206c5efab878c6c3" in "refs/heads/babel-collect-updates,refs/heads/backup,refs/heads/lerna-collect-updates,refs/heads/master,refs/heads/readme,refs/tags/v1.0.0…

@nicolo-ribaudo
Copy link
Member

Yeah sorry, we have fixed in on master

The type declarations for `@babel/parser` reference `@babel/types` but previously there was no explicit dependency on `@babel/parser`. This meant that TypeScript is unable to process `@babel/parser` unless `@babel/types` also happens to be installed. Furthermore, if installing using a stricter `node_modules` layout (like the one favored by `pnpm`), even if `@babel/types` is installed by another package `@babel/parser` will be unable to see it. With an explicit dependency this problem goes away.
@eventualbuddha
Copy link
Member Author

More unrelated failures?

Summary of all failing tests
 FAIL  packages/babel-preset-env/test/fixtures.js
  ● Test suite failed to run

    ENOMEM: not enough memory, read

      at ScriptTransformer._transformAndBuildScript (node_modules/@jest/transform/build/ScriptTransformer.js:443:43)
      at ScriptTransformer.transform (node_modules/@jest/transform/build/ScriptTransformer.js:513:25)
      at Object.<anonymous> (packages/babel-helper-fixtures/node_modules/lodash/_baseRest.js:1:43)

@babel-bot
Copy link
Collaborator

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

1 similar comment
@babel-bot
Copy link
Collaborator

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

@loganfsmyth
Copy link
Member

It seems like if we actually wanted to fix this, we'd have to make a new package that was just the type definitions or something.

Can we just ask people using TS to install @babel/types alongside the parser if they want to use the type definitions?

@eventualbuddha
Copy link
Member Author

You could go back to using type definitions from DefinitelyTyped. Or you could also include a comment in the type declarations file at the point where TS shows an error message that instructs users to add @babel/types to their package as a devDependency.

Both of these feel kinda janky. I'm curious what the actual harm with this is, besides possibly adding a copy of @babel/types to someone's node_modules folder. If that's all it is, is that so bad?

@eventualbuddha
Copy link
Member Author

Doesn't look like this is going to happen, so I'm closing.

@lock lock bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Jan 24, 2020
@lock lock bot locked as resolved and limited conversation to collaborators Jan 24, 2020
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