Allow shared nested addons to be properly discovered. #3335
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
/cc @lukemelia could you review? (since you championed the initial AddonDiscovery extraction) |
| }; | ||
| ``` | ||
|
|
||
| This should *not* pose a problem for the vast majority of addon's. |
There was a problem hiding this comment.
nit: remove the apostrophe
rwjblue
force-pushed
the
nested-addon-discovery
branch
from
38e072c
to
9a8b2c5
Compare
|
Looks good to me. Thanks @rwjblue! |
rwjblue
force-pushed
the
nested-addon-discovery
branch
2 times, most recently
from
ac2f6c7
to
f5fb943
Compare
|
There are some acceptance test failures that I am working through... |
Closed
Prior to these changes we always assumed that an addon was at the current package's (could be either the project or another addon) `node_modules/<addon-name>` path. This is true in many cases, but not all. Specifically, with more recent versions of NPM a package that is shared by a nested child package and an ancestor package will be deduped and used from that parent directory. Simple example: * Package A * Depends on Package B * Depends on Package C * Package C depends on Package B The folder structure in this case for more recent versions of NPM (2.0.0+) would be: ``` package-a/node_modules/package-b/* package-a/node_modules/package-c/* ``` But our prior logic expected: ``` package-a/node_modules/package-b/* package-a/node_modules/package-c/* package-a/node_modules/package-c/node_modules/package-b/* ``` --- This PR ensures that we use the `resolve` package (which follows the [documented node resolution strategy](http://nodejs.org/api/modules.html#modules_all_together)) to determine the path to a given addon. **Please note** that this PR also removes the ability to have an addon without any node-land JS (previously it was possible to have only a `package.json` and `vendor/` folder for example). I am not aware of this being relied upon, but it is documented as a breaking change in the changelog for 0.2.0.
rwjblue
force-pushed
the
nested-addon-discovery
branch
from
f5fb943
to
dfd2e0e
Compare
|
Just confirmed that this fixes the demo repo (https://github.com/alvincrespo/test-new-app) provided by @alvincrespo in #3240. |
rwjblue
added a commit
that referenced
this pull request
Feb 23, 2015
Allow shared nested addons to be properly discovered.
|
🍻 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Prior to these changes we always assumed that an addon was at the current package's (could be either the project or another addon)
node_modules/<addon-name>path. This is true in many cases, but not all. Specifically, with more recent versions of NPM a package that is shared by a nested child package and an ancestor package will be deduped and used from that parent directory.Simple example:
The folder structure in this case for more recent versions of NPM (2.0.0+) would be:
But our prior logic expected:
This PR ensures that we use the
resolvepackage (which follows the documented node resolution strategy) to determine the path to a given addon.Please note that this PR also removes the ability to have an addon without any node-land JS (previously it was possible to have only a
package.jsonandvendor/folder for example). I am not aware of this being relied upon, but it is documented as a breaking change in the changelog for 0.2.0.Closes #3240.