Peer and Optional Dependencies Support

[rwjblue]

rendered

[davewasmer]

I think npm used to install and / or enforce peerDependencies, if I remember correctly. If be curious what their rationale was for removing that functionality, and what makes ember-cli different such that it won't encounter the same issues.

[rwjblue]

I think npm used to install and / or enforce peerDependencies, if I remember correctly.

Indeed, npm@2 enforced peer dependencies, and then relaxed that to a simple warning in npm@3 and higher.

If be curious what their rationale was for removing that functionality, and what makes ember-cli different such that it won't encounter the same issues.

The only thing I can find (after only a few minutes of searching :wink: ) is this weekly update that says:

We will also be changing the behavior of peerDependencies in npm@3. We won’t be automatically downloading the peer dependency anymore. Instead, we’ll warn you if the peer dependency isn’t already installed. This requires you to resolve peerDependency conflicts yourself, manually, but in the long run this should make it less likely that you’ll end up in a tricky spot with your packages’ dependencies.

Which unfortunately, doesn't really give any rationale. I can only speculate based on anecdotal evidence that npm@3 switching to a more flat system and doing a slightly better job at de-duping meant that the node folks generally were ok with the old mantra of "just depend on what you need, don't worry if it gets duplicated". If that is the real reason in the end (remember I'm only speculating), it simply doesn't apply to us. Its pretty darn important that we don't ship two Ember versions or use two different template compilers (for example)...

[eriktrom]

npm team official statement is here: https://github.com/npm/npm/issues/11213#issuecomment-173415524

and yarn is debating peer deps too: https://github.com/yarnpkg/yarn/pull/4689

and optional deps in npm are just weird lol: https://github.com/npm/npm/issues/14185

(and i have questions, but i'll wait :) )

[jlami]

As I see it peerDependencies could be made to work with only the extra error message. While a peer dependency will be fulfilled by an npm install <package> without --save-dev, I feel that the normal way is to save it in your toplevel package.json. And that would automatically make ember-cli include it in the addon tree (I believe ember-cli does ignore an installed addon if it is not mentioned in the toplevel package.json, or has that behaviour changed?).

I think the real problem here is that npm install only reports a warning for missing peer deps (while npm ls does report an error). So if you don't have the peer dep in your package.json the extra error message from ember-cli would be enough to indicate to a user that he has to install the missing peerdependency.

For optional deps I don't really know what to do, don't have much experience with them. And reading the links above gives me the impression that it is not all that straight forward. Although the reasoning that an optional dep is not used by ember-cli until you include it in your app package.json could be a nice consistent approach.

--edit: After looking into the addon handling some more I don't know if I really know what I'm talking about. But I've added some more comments on the matter here: https://github.com/ember-cli/ember-cli/issues/7399#issuecomment-339433829

As far as why npm doesn't install peer deps anymore, I think this is more due to problems people had with multiple installations of libraries. As described in the first link of @eriktrom (the hard error) and also here: https://github.com/npm/npm/releases/tag/v3.0.0 But still I think the solution to make missing peer deps warnings was a bad decision that makes things harder to get correct.

install-peerdeps also seems encouraging. Although I miss the option to install all peer deps recursively.

[rwjblue]

I think the real problem here is that npm install only reports a warning for missing peer deps (while npm ls does report an error). So if you don't have the peer dep in your package.json the extra error message from ember-cli would be enough to indicate to a user that he has to install the missing peer dependency.

Yep, thats exactly the idea here.

But still I think the solution to make missing peer deps warnings was a bad decision that makes things harder to get correct.

I agree.

[rwjblue]

We discussed this at length in the ember-cli team meeting yesterday, the team is in favor of moving forward here (barring new info in comments / feedback / etc here) with the following changes to the RFC:

• Introduce a warning in the current beta (2.17 beta series) for scenarios that will become errors in 2.18. This will give folks a bit of time to digest and understand the impact and how to address the various scenarios that crop up.
• Do not instantiate and add an instance of the peer dependency into the depending addon's this.addons array if it were found. This is a feature we might want to add back, but that can be determined in a future RFC.
• Add an "opt-out" flag (likely via an environment variable) to change the hard error condition back into a warning.
• Add a new file in docs/dependencies.md (or other name) to link to from the warning and error messages. This file can evolve over time to describe the common issues and solutions for folks that hit the warning and/or errors.

I'll work to get this RFC updated with these points this weekend...

[jlami]

Good to hear there is some traction on this rfc.

Do not instantiate and add an instance of the peer dependency into the depending addon's this.addons array if it were found. This is a feature we might want to add back, but that can be determined in a future RFC.

How will this work with for example ember-cli-babel as a peer dependency? The existence of this in the addons is how ember-cli currently marks an addon for JS preprocessing as far as I could tell. I could not get the warning at https://github.com/ember-cli/ember-cli/blob/master/lib/models/addon.js#L1212 to go away without adding the pkg['peerDependencies'] into AddonDiscovery.dependencies(), which will make it end up in this.addons.

So maybe still load the addon for preprocessing, but don't add it onto the this.addons so it isn't included in flattening of the addon/app tree? Or change the way the 'marking for JS preprocessing' works all together?

[rwjblue]

How will this work with for example ember-cli-babel as a peer dependency? The existence of this in the addons is how ember-cli currently marks an addon for JS preprocessing as far as I could tell.

Yep, definitely is. We have a few options in this case:

• Implement custom discoverAddons to add its ember-cli-babel dep to the list.
• Expose a nice helper function to easily opt-in to discovering from peer dependencies.

Though, I am generally working (mostly on the concept, no code yet) on removing the "transpile at each layer" concept and making it so that each layer is responsible for emitting "ES" code (e.g. no pending proposals, etc). This would mean that addons wanting stage X features would still need to handle transpilation of those specific features, but overall most addons would not need this. The result of this would be that we can do a single transpile (using the apps transpiler addon) and have the ability to do other nice things (e.g. tree shaking, auto-import of ES modules from packages in node_modules, etc).

In summary, I think the primary concern that caused me to open this RFC is still addressed without setting up the peer dependency in this.addons, and that we have better solutions to solve the specific case of ember-cli-babel.

[jlami]

That sounds perfect. ember-cli-babel is indeed the exception here, so setting up this RFC to work for the other cases seems like the way to go.

[mazondo]

We're looking at leveraging peerDependencies in our app, does this RFC imply we'll hit issues, or are we good to go there?

[rwjblue]

Closed as part of the migration to emberjs/rfcs, will reopen over in emberjs/rfcs...