Skip to content

Move @babel/core to peerDependencies to resolve peer dependency warnings and errors#452

Merged
bertdeblock merged 1 commit into
emberjs:masterfrom
NullVoxPopuli:move-babel-core-to-peer
Oct 1, 2022
Merged

Move @babel/core to peerDependencies to resolve peer dependency warnings and errors#452
bertdeblock merged 1 commit into
emberjs:masterfrom
NullVoxPopuli:move-babel-core-to-peer

Conversation

@NullVoxPopuli

Copy link
Copy Markdown
Contributor

…confusion

Following the advice here: ember-cli/ember-cli#9934 (comment)

The eventual perceived fix for users is that when using strict package managers (npm 8, yarn2+, pnpm, etc) is that when they are told to specify a missing peer, @babel/core, the @babel/core they specify will be the one used by ember-cli-babel.

@babel/core will eventually be added to the app and addon blueprints in ember-cli/ember-cli#9934

@NullVoxPopuli

Copy link
Copy Markdown
Contributor Author

I have 8 failures when I run the tests -- but those 8 failures also exist on master so.... that's fun

@ef4

ef4 commented Aug 10, 2022

Copy link
Copy Markdown
Collaborator

You need a devDependency too.

(This is an example of the confusing fallout from making v1 addons be their own test-app too, in a single package. The package.json has two meanings. It needs to do the work that a consuming app would do (put @babel/core in devDependencies) while also doing the work of the addon (put @babel/core in peerDependencies).

@NullVoxPopuli

Copy link
Copy Markdown
Contributor Author

devDependency is there

@ef4

ef4 commented Aug 10, 2022

Copy link
Copy Markdown
Collaborator

Oops, sorry, somehow I looked right at it and didn't see it. 😛

@ef4

ef4 commented Sep 8, 2022

Copy link
Copy Markdown
Collaborator

I think we can make the argument that this is a bugfix instead of breaking change because people were already experiencing peer dep warnings and/or failures depending on their package manager, so documenting the fact that you already needed this peer dep to avoid those issues is only a bug fix.

@bertdeblock

Copy link
Copy Markdown
Member

This seems ready to go?

@ef4

ef4 commented Sep 15, 2022

Copy link
Copy Markdown
Collaborator

It was pointed out to me that this really is probably breaking, because it's not the addition to peerDependencies that is the big problem, it's the removal from dependencies.

@ef4

ef4 commented Sep 15, 2022

Copy link
Copy Markdown
Collaborator

I still think we should do it, we can make this ember-cli-babel 8.

I know a traditional reason not to do a major release here was to save it for a babel major release, but this change means we're decoupled from the babel version anyway.

@NullVoxPopuli

Copy link
Copy Markdown
Contributor Author

Sounds good to me -- as long as we can specify in the release notes that resolutions / overrides can still be used to collapse all ember-cli-babel versions down to 1 version in folks dep-trees, and v8 would still be compatible with their projects, provided they add @babel/core (and likely meet other criteria we move as a part of the major (node engines, etc))

@ef4

ef4 commented Sep 28, 2022

Copy link
Copy Markdown
Collaborator

Discussed with ember-cli core team and agreed to merge this, once we see it run CI. For some reason that hasn't happened yet. Investigating.

Also, the plan to release this is tracked in #453 which I will fill out with more details.

@ef4 ef4 mentioned this pull request Sep 28, 2022
10 tasks
@bertdeblock
bertdeblock force-pushed the move-babel-core-to-peer branch from a239c07 to 83fa078 Compare October 1, 2022 14:32
@bertdeblock

bertdeblock commented Oct 1, 2022

Copy link
Copy Markdown
Member

@NullVoxPopuli I added a test commit and dropped it again to trigger CI, hope you don't mind!

@bertdeblock bertdeblock added the bug label Oct 1, 2022
@bertdeblock bertdeblock changed the title Move @babel/core to peerDependencies to reduce dependency resolution … Move @babel/core to peerDependencies to resolve peer dependency warnings and errors Oct 1, 2022
@bertdeblock

Copy link
Copy Markdown
Member

CI is green, so I'm merging!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants