Resolve peers from Glint and/or ember-source #101
Conversation
… (and ember-source would be required the moment you import anything anyway)
NullVoxPopuli
changed the title
There was a problem hiding this comment.
Looks good in general, but...
Tbh, I have more than one addon where I historically never cared about peerDeps and put just everything into deps (not ember-source of course, but @glimmer/component for example). I see that the v1 addon blueprint now also has a peer dependency on ember-source, but not for @glimmer/component, I guess because not every addon necessarily will have expose components (but most/a lot will).
Which brings up the question: should we do this here?
You did this specifically because we chose to add Glint types by default, and they rely on the presence of @glimmer/component, right? If so, we only have glint packages as devDependencies, so to satisfy the peer dependency we could just add @glimmer/component to our devDependencies as well, right?
Second, however we decide on this, I think we need better guidance for addon authors on this topic (what is a dependency, what a peer, all with examples specific to the Ember ecosystem). Especially should we decide to not declare @glimmer/component a peer by default here. People - like me 🙈 - will just think, ok, I am actually importing @glimmer/component from exposed run-time code, so I'll just add it to my dependencies.
Of course out of scope for this PR, so sorry for diverging a bit, but wanted to leave this here before I forget... Especially as we have a learning team member here among the reviewers! 😬
ember-source also has declared a peer on Without this change: ember addon peer-test -b ../NullVoxPopuli/addon-blueprint-2/ --skip-npm --pnpm
cd peer-test
pnpm i
# ... ->
WARN Issues with peer dependencies found
peer-test
├─┬ ember-source 4.10.0
│ ├── ✕ missing peer @glimmer/component@^1.1.2This may be an incorrect thing for ember-source to do, as |
…-source, @glimmer/component
| @@ -25,6 +25,9 @@ | |||
| "test": "echo 'A v2 addon does not have tests, run tests in test-app'", | |||
| "prepack": "rollup --config" | |||
| }, | |||
| "peerDependencies": { | |||
There was a problem hiding this comment.
@simonihmig now only ember-source is a peer.
you are correct that it doesn't make sense for us to include @glimmer/component as a peer by default (as many utility libraries may not care about @glimmer/component at all!)
| @@ -34,6 +37,7 @@ | |||
| "@babel/plugin-proposal-class-properties": "^7.16.7", | |||
| "@babel/plugin-proposal-decorators": "^7.17.0", | |||
| "@babel/plugin-syntax-decorators": "^7.17.0", | |||
| "@glimmer/component": "^1.1.2", | |||
There was a problem hiding this comment.
this devDep satisfies the peer requirements from ember-source and glint (when used)
NullVoxPopuli
changed the title
|
Because everything in ember-source doesn't really resolve normally anyway, there's not much functional reason to have the peerDep on ember-source. The main reason people are forced to add it is if they want to say And because package managers do confusing and painful things with peerDeps, I'm hesitant to encourage every v2 addon to add this unused-but-likely-to-break-your-monorepo peerDep right now. |
… (and ember-source would be required the moment you import anything anyway)
@glimmer/componentis required by Glintember-sourceis required by nearly everything 🙃 (so this addition is more preemptive than anything)Partially resolves #100 and #77