[ENHANCEMENT] Support custom node_module paths #3771

jakehow · 2015-04-03T05:18:05Z

This enables custom node_module paths through an environment variable EMBER_NODE_PATH. Places that previously constructed node_modules paths manually now use a shared function nodeModulesPath.

Because this behavior is implicit right now I am not 100% sure if the tests I wrote capture all of the desired behavior.

This PR is intended to resolve #3735

cc @stefanpenner

stefanpenner · 2015-04-03T06:27:35Z

looks like windows is unhappy

jakehow · 2015-04-03T21:57:00Z

@stefanpenner I think my last variation should pass (looks like builds got really backed up and someone cancelled a bunch of them). If there is a better suggestion as to how to handle this on windows I am open to hear it (I currently do not have a local way to test on windows).

stefanpenner · 2015-04-03T21:57:58Z

lib/cli/node-modules-path.js

+  var nodePath = process.env.EMBER_NODE_PATH;
+  var contextPath = path.resolve(context);
+
+  if(nodePath) {


stefanpenner · 2015-04-03T22:00:27Z

do all the NPM commands npm install npm link and ember install:npm correctly observe these changes?

jakehow · 2015-04-03T22:09:00Z

@stefanpenner fixed whitespace/formatting issues, let me know if anything else is off

npm commands will not be effected by this in any way, the environment variable is specific to ember-cli

It would be the responsibility of the developer to run npm install in the correct directory if they want to set this variable.

The ember-cli:install tasks appears to call npm tasks directly so there should be no behavior change here either. There are no paths passed as options to these tasks or anything like that.

stefanpenner · 2015-04-03T22:15:46Z

I will try to spend some time understanding the docker issue correct. I am hesitant to accept more complexity here, without fully understanding the problem it aims to solve.

jakehow · 2015-04-03T22:21:35Z

@stefanpenner let me know if you need any help with this, I will also bang on this next week with @mixonic (he will be in our office all week). The heart of the issue is really very simple in that ember-cli is hardcoding the node_module dependency paths which makes the runtime environment inflexible. The docker problems are a symptom of this issue.

There is a secondary benefit that comes from changing this which is that the behavior of dependency loading is currently implicit and harder to understand than it should be since there are no explicit requirements or tests. I actually think the bower_components path loading code all works correctly for the docker scenario based on my current understanding, but it would still be valuable to break that out into its own function as well so it can be explicitly tested and its behavior specified.

stefanpenner · 2015-04-03T22:24:17Z

lib/cli/node-modules-path.js

+var path  = require('path');
+
+module.exports = function(context) {
+  var nodePath = process.env.EMBER_NODE_PATH;


should we just try to observer NODE_PATH directly then? I imagine this would align ember-cli with node's own behavior more closely? Or is this going to far?

@steffanpenner NODE_PATH brings extra baggage with it. It will try to load globally installed modules and node_modules installed in $HOME which we might want to avoid. My goal here is t be able to say I have a clean controlled node_modules path but it is not necessarily at the project root.

sounds reasonable, thanks.

Default NODE_PATH behavior is described here. https://nodejs.org/api/modules.html#modules_loading_from_the_global_folders

If this value is unset we could actually do whatever we wanted with it, but I think doing something different than what's described in the node docs would be confusing to developers familiar with the default behavior, so keeping our own envvar seems like the best option.

mixonic · 2015-04-04T00:37:09Z

I actually think the bower_components path loading code all works correctly for the docker scenario based on my current understanding, but it would still be valuable to break that out into its own function as well so it can be explicitly tested and its behavior specified.

It sounds like bower will be going away in the future, so if it is working well for now it is likely safe to punt making any changes.

jakehow · 2015-04-04T05:22:43Z

@mixonic sounds good, only benefit would be it would be in one nice little spot to rip out when this happens, and we would have specs which might make that transition easier.

mixonic · 2015-04-06T14:42:44Z

lib/cli/node-modules-path.js

+    var nodeModulesPath = path.resolve(nodePath);
+
+    if (contextPath.indexOf(nodeModulesPath) === 0) {
+      return path.resolve(contextPath,'node_modules');


This seems to use contextPath when it is more specific than nodeModulesPath.

For example given a contextPath of /Projects/cat-time and a nodeModulesPath of /Projects, the /Projects/cat-time directory would be used (/Projects has an index of 0 on /Projects/cat-time).

This seems backwards? And perhaps we should wrap this in a small semantic helper called hasSubdirectoryOf etc.

@mixonic I think this is the correct behavior based on what the code currently does from my reading. If node_modules path is /Projects/node_modules and a contextPath of /Projects/node_modules/my-add-on is passed in, we want to return /Projects/node_modules/my-add-on/node_modules

Right now this is reflected in the tests in this way. Happy to break it out into an intention revealing function though.

Right, we discussed. The more specific path is correct, as it may be a specific subdirectory/nested dep in node_modules/

bantic · 2015-04-07T15:38:55Z

This has been updated to use a isSubdirectoryOf intention-revealing method inside the node-modules-path utility, and had some documentation added. It's also been squashed and rebased against latest master. This should be good to go, now.

mixonic · 2015-04-07T16:45:50Z

Did some Real World testing, and this requires a few ecosystem changes:

ember-cli-dependency-checker must be improved to use the same node_modules as the project itself.
Master of rwjblue/ember-cli-ic-ajax is required. Fixes a bug where paths were joined and not resolved.
ember-cli-htmlbars needs to correctly resolve paths instead of join, as submitted in bowerDirectory can be an absolute path ember-cli-htmlbars#49

bantic · 2015-04-07T18:43:48Z

lib/cli/node-modules-path.js

+// A utility function for determining what path an addon may be found at. Addons
+// will only be resolved in the project's local `node_modules/` directory. This
+// is in contrast to a plain `require('some-module')` lookup, which would resolve
+// a library according to the paths in NODE_MODULES_PATH.


should be NODE_PATH

bantic · 2015-04-07T19:50:23Z

Here's a test app that works with its node_modules directory outside of the project, along with instructions:

https://github.com/bantic/test-app

It removes the ember-cli-dependency-checker, but we'll update the dep checker to be more tolerant of a different node_modules location.

mixonic · 2015-04-07T22:19:11Z

Sanity checked and good to go. /cc @rwjblue the contains nodeModulesPath as a property. It is a fully resolved path (unlike bowerDirectory).

stefanpenner · 2015-04-07T22:58:39Z

lib/cli/node-modules-path.js

+// * https://nodejs.org/api/modules.html#modules_all_together
+//
+// By requiring with an absolute path this logic is bypassed.
+//


can this be converted into the YUIdoc form, this will match other documented functions.

we are slowly making progress on improving doc coverage: https://inch-ci.org/github/ember-cli/ember-cli?pending_build=47203

Yeah I'll update that tonight or tomorrow morning

On Apr 7, 2015, at 6:58 PM, Stefan Penner notifications@github.com wrote:

In lib/cli/node-modules-path.js:

+function isSubdirectoryOf(parentPath, possibleChildPath) {

return possibleChildPath.length > parentPath.length &&

possibleChildPath.indexOf(parentPath) === 0;
+}

+// A utility function for determining what path an addon may be found at. Addons
+// will only be resolved in the project's local node_modules/ directory. This
+// is in contrast to a plain require('some-module') lookup, which would resolve
+// a library according to the paths in NODE_PATH.
+//
+// A description of node's lookup logic can be found here:
+//
+// * https://nodejs.org/api/modules.html#modules_all_together
+//
+// By requiring with an absolute path this logic is bypassed.
+//
can this be converted into the YUIdoc form, this will match other documented functions.

we are slowly making progress on improving doc coverage: https://inch-ci.org/github/ember-cli/ember-cli?pending_build=47203

—
Reply to this email directly or view it on GitHub.

bantic · 2015-04-08T15:29:43Z

@stefanpenner updated the comments to be yuidoc style.

We also have a PR to ember-cli-dependency-checker pending that will update it to respect these path changes. The test-app is updated to use it. It now runs fine with a node_modules directory outside of the project root and stock ember-cli addons except that we are using customized-and-PR'd versions for dependency checker, ic-ajax and htmlbars.

rwjblue · 2015-04-08T15:33:24Z

@bantic - ember-cli-ic-ajax@0.1.2 and ember-cli-htmlbars@0.7.5 are released, and should solve the need to run custom versions, please confirm?

bantic · 2015-04-08T16:11:40Z

@rwjblue confirmed! Thanks. Updated the test app to use these versions and it works great.

mixonic · 2015-04-09T15:05:24Z

@stefanpenner I think we're waiting on a +1 and merge from you. Iterating on changes this week is ideal, let us know if we need to change anything.

stefanpenner · 2015-04-09T16:09:41Z

kicking appveyor.

I don't have time to thorough review this this week, so I will take your confidence as sufficient in merging once appveyor is happy. I don't want to block the train, but if we discover issues down the road I will pester the 3 of you ;)

ember-cli will use the value in env.EMBER_NODE_PATH if it is provided to look up addons when `require`-ing them. This allows a user to have their `node_modules` path outside of the project root. Previously, the ember-cli custom require code assumed that `node_modules` is always in the project's root.

This uses the cli utility helper in node-modules-path to determine the nodeModulesPath. Also modifies private method `AddonDiscovery.discoverFromDependencies` to get the nodeModules path from the project (or addon if inside AddonDiscovery.discoverChildAddons) as its second argument. This property is intended to provide a way for 3rd-party addons (like ember-cli-dependency-checker) to know the project's node modules path without having to discover it in an ad hoc way.

mixonic · 2015-04-15T01:28:36Z

rebased and force pushed.

[ENHANCEMENT] Support custom node_module paths

stefanpenner added needs-investigation labels Apr 3, 2015

stefanpenner reviewed Apr 3, 2015
View reviewed changes

stefanpenner self-assigned this Apr 3, 2015

mixonic reviewed Apr 6, 2015
View reviewed changes

bantic force-pushed the node-module-path-support branch 2 times, most recently from e7a7abc to 86b79a8 Compare April 7, 2015 15:37

bantic force-pushed the node-module-path-support branch from 86b79a8 to 2fe39fa Compare April 7, 2015 15:43

mixonic force-pushed the node-module-path-support branch from 2fe39fa to 77f0b84 Compare April 7, 2015 16:19

mixonic force-pushed the node-module-path-support branch from 77f0b84 to 4ff0f71 Compare April 7, 2015 17:14

bantic reviewed Apr 7, 2015
View reviewed changes

mixonic force-pushed the node-module-path-support branch from 4ff0f71 to c5c3f8d Compare April 7, 2015 19:36

bantic force-pushed the node-module-path-support branch from 0caf386 to b249454 Compare April 7, 2015 21:08

mixonic force-pushed the node-module-path-support branch 2 times, most recently from f0d0737 to 096178b Compare April 7, 2015 22:05

mixonic mentioned this pull request Apr 7, 2015

Add support for npm-shrinkwrap.json quaertym/ember-cli-dependency-checker#26

Merged

stefanpenner reviewed Apr 7, 2015
View reviewed changes

bantic force-pushed the node-module-path-support branch 3 times, most recently from a64df18 to 4c4809e Compare April 8, 2015 15:24

mixonic force-pushed the node-module-path-support branch from 4c4809e to 30241ac Compare April 9, 2015 17:21

bantic force-pushed the node-module-path-support branch from 30241ac to 1509caf Compare April 9, 2015 17:45

bantic and others added 4 commits April 14, 2015 21:27

fix incorrect test setup for project unit test

8cb2353

Change comments to yuidoc style

b844284

mixonic force-pushed the node-module-path-support branch from 1509caf to b844284 Compare April 15, 2015 01:28

stefanpenner added a commit that referenced this pull request Apr 23, 2015

Merge pull request #3771 from jakehow/node-module-path-support

dba71be

[ENHANCEMENT] Support custom node_module paths

stefanpenner merged commit dba71be into ember-cli:master Apr 23, 2015

jakehow deleted the node-module-path-support branch April 23, 2015 21:21

bantic mentioned this pull request May 11, 2015

Use ember-cli project nodeModulesPath when available quaertym/ember-cli-dependency-checker#31

Merged

nathanhammond mentioned this pull request Sep 22, 2016

ember-cli does not respect bower_directory environment variable #6290

Closed

ef4 mentioned this pull request Oct 26, 2017

Resolve node modules correctly #7401

Merged

krukid mentioned this pull request Nov 5, 2018

Cannot boot with external node_modules #8166

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ENHANCEMENT] Support custom node_module paths #3771

[ENHANCEMENT] Support custom node_module paths #3771

jakehow commented Apr 3, 2015

stefanpenner commented Apr 3, 2015

jakehow commented Apr 3, 2015

stefanpenner Apr 3, 2015

stefanpenner commented Apr 3, 2015

jakehow commented Apr 3, 2015

stefanpenner commented Apr 3, 2015

jakehow commented Apr 3, 2015

stefanpenner Apr 3, 2015

jakehow Apr 3, 2015

stefanpenner Apr 3, 2015

jakehow Apr 4, 2015

mixonic commented Apr 4, 2015

jakehow commented Apr 4, 2015

mixonic Apr 6, 2015

jakehow Apr 6, 2015

mixonic Apr 7, 2015

bantic commented Apr 7, 2015

mixonic commented Apr 7, 2015

bantic Apr 7, 2015

bantic commented Apr 7, 2015

mixonic commented Apr 7, 2015

stefanpenner Apr 7, 2015

bantic Apr 7, 2015

bantic commented Apr 8, 2015

rwjblue commented Apr 8, 2015

bantic commented Apr 8, 2015

mixonic commented Apr 9, 2015

stefanpenner commented Apr 9, 2015

mixonic commented Apr 15, 2015

[ENHANCEMENT] Support custom node_module paths #3771

[ENHANCEMENT] Support custom node_module paths #3771

Conversation

jakehow commented Apr 3, 2015

stefanpenner commented Apr 3, 2015

jakehow commented Apr 3, 2015

Choose a reason for hiding this comment

stefanpenner commented Apr 3, 2015

jakehow commented Apr 3, 2015

stefanpenner commented Apr 3, 2015

jakehow commented Apr 3, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mixonic commented Apr 4, 2015

jakehow commented Apr 4, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bantic commented Apr 7, 2015

mixonic commented Apr 7, 2015

Choose a reason for hiding this comment

bantic commented Apr 7, 2015

mixonic commented Apr 7, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bantic commented Apr 8, 2015

rwjblue commented Apr 8, 2015

bantic commented Apr 8, 2015

mixonic commented Apr 9, 2015

stefanpenner commented Apr 9, 2015

mixonic commented Apr 15, 2015