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

Fix errors reported by strict peer checking #9934

Closed

Conversation

NullVoxPopuli
Copy link
Contributor

Resolves: #9933

@ef4
Copy link
Contributor

ef4 commented Jun 8, 2022

The problem is this doesn't reliably fix the problem. It assumes that hoisting will work out and that some deeply nested packages will be able to resolve the app's copy of @babel/core, but that is not always true.

Given an error like:

├─┬ @ember/test-helpers
│ └─┬ ember-destroyable-polyfill
│   └─┬ ember-compatibility-helpers
│     └─┬ babel-plugin-debug-macros
│       └── ✕ missing peer @babel/core@^7.0.0-beta.42

In order to satisfy a peer dependency, the peer of babel-plugin-debug-macros needs to have @babel/core, which means ember-compatibility-helpers needs to have either a dependency on @babel/core (which is probably wrong) or a peerDependency on @babel/core, which implies recursively that ember-destroyable-polyfill and @ember/test-helpers should also have peerDependencies on @babel/core all the way up to the app's devDependency on @babel/core.

@ef4
Copy link
Contributor

ef4 commented Jun 8, 2022

Also, I think having an unused copy of @babel/core in the app is bad, and so to address that we should change ember-cli-babel so it moves @babel/core from dependencies to peerDependencies. That would make the apps copy actually matter.

@ef4
Copy link
Contributor

ef4 commented Jun 8, 2022

This recursive peer dependencies situation actually makes sense when you consider that it requires each addon to document what range of babel it works under, and those requirements necessarily recurse (if your addon depends on another addon that only works in a certain babel range, your addon also only works in that babel range).

@NullVoxPopuli
Copy link
Contributor Author

🤔 that makes sense -- thanks for explaining the situation!

so given these errors:

├─┬ @ember/test-helpers
│ └─┬ ember-destroyable-polyfill
│   └─┬ ember-compatibility-helpers
│     └─┬ babel-plugin-debug-macros
│       └── ✕ missing peer @babel/core@^7.0.0-beta.42
├─┬ @glimmer/component
│ └─┬ ember-cli-typescript
│   └─┬ @babel/plugin-transform-typescript
│     ├── ✕ missing peer @babel/core@^7.0.0-0
│     ├─┬ @babel/helper-create-class-features-plugin
│     │ └── ✕ missing peer @babel/core@^7.0.0
│     └─┬ @babel/plugin-syntax-typescript
│       └── ✕ missing peer @babel/core@^7.0.0-0
├─┬ ember-data
│ └─┬ @ember-data/adapter
│   └─┬ @ember-data/private-build-infra
│     ├─┬ @babel/plugin-transform-block-scoping
│     │ └── ✕ missing peer @babel/core@^7.0.0-0
│     └─┬ babel-plugin-debug-macros
│       └── ✕ missing peer @babel/core@^7.0.0
└─┬ ember-load-initializers
  └─┬ ember-cli-typescript
    ├─┬ @babel/plugin-proposal-class-properties
    │ └── ✕ missing peer @babel/core@^7.0.0-0
    └─┬ @babel/plugin-transform-typescript
      └── ✕ missing peer @babel/core@^7.0.0-0

is it best to propagate the @babel/core through all of these?
or should we:
add @babel/core as a dep (with wide range) to:

  • ember-compatibility-helpers
  • ember-cli-typescript
  • @ember-data/private-build-infra

atm, I'm not sure if having @babel/core in apps makes sense, given that all of babel is abstracted away in ember-cli-babel --- unless there is eventual plan to move the babel deps to the app (which... I wouldn't want exactly.. low level build tooling is super rough when first learning JS)

@ef4
Copy link
Contributor

ef4 commented Jun 14, 2022

We're not free to pick and choose here, babel has decided that plugins are peerDeps of @babel/core so there's really only one correct way to do that and it's the things I said above.

@NullVoxPopuli
Copy link
Contributor Author

I don't understand how adding @babel/core everywhere is a rereq to this tho -- can you expand on that?

adding @babel/core to the app blueprint doesn't make it an unused peer deep -- pnpm is using it.

@ef4
Copy link
Contributor

ef4 commented Aug 10, 2022

Ask yourself: why do babel plugins have a peerDep on @babel/core in the first place?

If they need to access something from @babel/core they can just have their own dependency on it. "With wide range" as you said above. That would save all consumers from worrying about it at all.

But that's not what they do. Why? Why do they declare the peerDep and force everybody to deal with it?

It's because getting access to the copy of @babel/core that is actually doing the transpiling is important to them.

Now, in an ember app, which copy of @babel/core is actually doing the transpiling? It's the one that is a dependency of ember-cli-babel.

So to actually give the plugins what they're asking for, we need to make sure that if any of them does require('@babel/core') it gets back the exact same copy that ember-cli-babel gets when it says require('@babel/core').

This is why I said above:

we should change ember-cli-babel so it moves @babel/core from dependencies to peerDependencies. That would make the app's copy actually matter.

If you just add @babel/core to the app without first changing ember-cli-babel, ember-cli-babel will keep using its own copy and ignoring the app's copy, while all the plugins are resolving the app's copy. It's true that pnpm will stop complaining to you, but that's because it doesn't have any way of knowing that ember-cli-babel is sneakily using its own separate copy to do all the real work.

@NullVoxPopuli
Copy link
Contributor Author

Gotchya, thanks! I have a flurry of prs to make then!

@ef4
Copy link
Contributor

ef4 commented Aug 10, 2022

The main one to start with is ember-cli-babel.

@bertdeblock
Copy link
Contributor

@NullVoxPopuli Going to close this one in favor of #10141 if that's okay.

@NullVoxPopuli
Copy link
Contributor Author

of course!

@NullVoxPopuli NullVoxPopuli deleted the resolve-peer-errors branch January 27, 2023 22:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

peer errors with app blueprint
3 participants