Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(#1875): Support npm linked libraries #2291

Merged
merged 1 commit into from Oct 10, 2016
Merged

fix(#1875): Support npm linked libraries #2291

merged 1 commit into from Oct 10, 2016

Conversation

serhiisol
Copy link
Contributor

@serhiisol serhiisol commented Sep 22, 2016

This PR fixes the problem with linked packages (#1875).

Originally I've used http://webpack.github.io/docs/troubleshooting.html#npm-linked-modules-doesn-t-find-their-dependencies official documentation. But for webpack2 the team has changed also configuration object, which doesn't work like described in the article.

So I've found another release note related to configuration object - https://gist.github.com/sokra/27b24881210b56bbaff7#resolving-options

The most important is Resolving options section.

So we have to change our configuration object from:

module.exports = {
  resolve: { fallback: path.join(__dirname, "node_modules") },
  resolveLoader: { fallback: path.join(__dirname, "node_modules") }
};

to:

module.exports = {
  resolve: { 
    modules: [ path.join(__dirname, "node_modules") ]
  }
};

Closes #1875

@filipesilva
Copy link
Contributor

@TheLarkInn can you review?

@clydin
Copy link
Member

clydin commented Oct 4, 2016

The new equivalent of fallback should be the following:

resolve: {
  modules: ['node_modules', path.resolve(projectRoot, 'node_modules')]
}

@clydin
Copy link
Member

clydin commented Oct 9, 2016

@filipesilva, this PR replaces the module resolution whereas the webpack recommended config adds a fallback.

@filipesilva
Copy link
Contributor

@clydin, good eye, thanks for the heads-up!

@@ -33,7 +33,8 @@ export function getWebpackCommonConfig(
return {
devtool: 'source-map',
resolve: {
extensions: ['.ts', '.js']
extensions: ['.ts', '.js'],
modules: [ path.join(projectRoot, 'node_modules') ]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you use modules: ['node_modules', path.resolve(projectRoot, 'node_modules')] instead to have a fallback?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See #2291 (comment) for details.

Copy link

@mmolhoek mmolhoek Oct 10, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@filipesilva, @serhiisol: FYI: when I use
modules: ['node_modules', path.resolve(projectRoot, 'node_modules')]
it does not work, while when I use
modules: [ path.join(projectRoot, 'node_modules') ]
it does :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mmolhoek, do you have an example?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@clydin it has still that trouble

metadata_resolver.js:245Uncaught Error: Unexpected value '[object Object]' imported by the module 'SupportChatModule'

or similar to it,

@mmolhoek try this:

modules: [path.resolve(projectRoot, 'node_modules'), 'node_modules']

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@clydin, not needed, @serhiisol gave the right modification

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@TheLarkInn can you weigh as a webpack core member?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

modules: [ path.join(projectRoot, 'node_modules') ] is correct. Requires absolute path to module directory as described.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sync branch with master and LGTM pending CI.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@filipesilva
Copy link
Contributor

Waiting for CI but added approval.

texel pushed a commit to splice/angular-cli that referenced this pull request Nov 3, 2016
fiznool added a commit to fiznool/ionic-app-scripts that referenced this pull request Nov 11, 2016
This patch fixes support for npm linked libraries.

More information can be found at angular/angular-cli#2291
danbucholtz pushed a commit to ionic-team/ionic-app-scripts that referenced this pull request Nov 18, 2016
This patch fixes support for npm linked libraries.

More information can be found at angular/angular-cli#2291
@sachanacar
Copy link

Hi all,

I'm also having this error but I don't use webpack, instead I use SystemJS and Vagrant. Do you know how I could emulate the fix you mentioned for these instead?

Basically doing this:

resolve: { modules: ['node_modules', path.resolve(projectRoot, 'node_modules')] }

But for people not using Webpack.

Thank you for your help!

@blingwang
Copy link

It seems we also need this fix in webpack-build-test file.
#3463

@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 11, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support npm linked libraries
8 participants