Resolve node modules correctly #7401
Conversation
| for (let depName in this.dependencies()) { | ||
| try { | ||
| this.resolveSync(depName); | ||
| return true; |
There was a problem hiding this comment.
Looks like it will return true on the first found package. Is it intentional or should we check all the deps?
There was a problem hiding this comment.
Ah sorry, missed you comment few lines above
lib/models/addon.js
Outdated
| let nodeModulesPath = require('node-modules-path'); | ||
| let value = nodeModulesPath(this.root); | ||
| this._nodeModulesPath = { value }; | ||
| this.ui.writeDeprecateLine('An addon is trying to access addon.nodeModulesPath. This is not a reliable way to discover npm modules. Instead, consider using require("resolve").sync(something, { basedir: addon.root })'); |
There was a problem hiding this comment.
We should include the stack here (or add an extra debug level) so folks have a chance at tracking this down
There was a problem hiding this comment.
We could also open issues for the addons we can see:
https://emberobserver.com/code-search?codeQuery=nodeModulesPath
There was a problem hiding this comment.
I added a stack-based message to identify the culprit.
| @param {String} file File to resolve | ||
| @return {String} Absolute path to file | ||
| */ | ||
| resolve(file) { |
There was a problem hiding this comment.
Thoughts on adding a private API deprecation here also? Might not matter though because I didn't find any usages of project.resolve in emberobserver...
There was a problem hiding this comment.
I looked too, I couldn't find anything. I vote for leaving it out and you can assign the future issue to me when something blows up. :-)
| @@ -323,7 +329,7 @@ class Project { | |||
| */ | |||
| resolveSync(file) { | |||
| return resolve.sync(file, { | |||
| basedir: this.root, | |||
| basedir: process.env.EMBER_NODE_PATH || this.root, | |||
There was a problem hiding this comment.
I don't grok what this environment variable is about, can you explain?
There was a problem hiding this comment.
This is only used in one addon (per https://emberobserver.com/code-search?codeQuery=EMBER_NODE_PATH) and that looks like a copy-paste of ember-cli's own tests?
There was a problem hiding this comment.
I would be totally happy to eliminate this feature, I just can't tell who's depending on it. I think it's more of an app feature than an addon feature.
It was added here: #3771
I think it was a roundabout way of doing what this PR is also doing: allow people to put their node_modules in the other places that you would expect to work based on node behavior.
There was a problem hiding this comment.
Yeah, agreed. Would you mind triggering a deprecation if we detect that process.env.EMBER_NODE_PATH is set?
There was a problem hiding this comment.
Lets take this as a separate issue (and not block this PR on it)...
ember-cli attempts to provides its own module resolution semantics which are only a subset of node's. These resolution semantics are complex and we spend many lines of code implementing them, all for no benefit over node's native resolution algorithm. As a side effect of this behavior, we cannot take advantage of nice things like [yarn workspaces](ember-cli#7335). This PR refactors ember-cli to use complete node modules resolution (as reimplemented in userspace by the [resolve](http://npmjs.com/package/resolve) package. As far as I can tell, I have preserved all existing behavior, including the (dubious and ripe for deprecation) EMBER_NODE_PATH environment variable. The entire `node-modules-path` package can be dropped as a dependency once we decide to stop offering the (private-but-used-by-addons) `project.nodeModulesPath` property. I think addons should be told to use `project.require` (already public) or `project.resolveSync` (would need to be marked public) wherever they were previously using `project.nodeModulesPath`. It's just fundamentally not correct to try to encode the modules location as a single path.
c52708d
to
9a35faf
Compare
|
Thank you @ef4! |
|
Today I've converted our solution to monorepo. It works great with current master. |
|
That's great! I'm surprised it worked actually, because there are some standard addons that still need to be updated like ember-cli-dependency-checker. |
|
This is working great for us, with some minor tweaks. PR(s) coming |
| // Blueprint tests set this flag. | ||
| return true; | ||
| } | ||
| for (let depName in this.dependencies()) { |
based off of ember-cli/ember-cli#7401 `nodeModulesPath` has been depreciated, and `resolveSync` is a preferred alternative. `nodeModulesPath` has been removed in Ember Cli 3.x and it's use here breaks 3.x builds.
ember-cli attempts to provides its own module resolution semantics which are only a subset of node's. These resolution semantics are complex and we spend many lines of code implementing them, all for no benefit over node's native resolution algorithm.
As a side effect of this behavior, we cannot take advantage of nice things like yarn workspaces.
This PR refactors ember-cli to use complete node modules resolution (as reimplemented in userspace by the resolve package. As far as I can tell, I have preserved all existing behavior, including the (dubious and ripe for deprecation) EMBER_NODE_PATH environment variable.
The entire
node-modules-pathpackage can be dropped as a dependency once we decide to stop offering the (private-but-used-by-addons)project.nodeModulesPathproperty.I think addons should be told to use
project.require(already public) orproject.resolveSync(would need to be marked public) wherever they were previously usingproject.nodeModulesPath. It's just fundamentally not correct to try to encode the modules location as a single path.