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

Regression with npm dependencies #3023

Closed
ShadowManu opened this issue Nov 4, 2016 · 8 comments
Closed

Regression with npm dependencies #3023

ShadowManu opened this issue Nov 4, 2016 · 8 comments
Labels
needs: investigation Requires some digging to determine if action is needed P1 Impacts a large percentage of users; if a workaround exists it is partial or overly painful

Comments

@ShadowManu
Copy link

Please provide us with the following information:

OS?

Windows 7, 8 or 10. Linux (which distribution). Mac OSX (Yosemite? El Capitan?)

Linux x64

Versions.

Please run ng --version. If there's nothing outputted, please run in a Terminal: node --version and paste the result here:

Could not start watchman; falling back to NodeWatcher for file system events.
Visit http://ember-cli.com/user-guide/#watchman for more info.
angular-cli: 1.0.0-beta.19-3
node: 6.9.1
os: linux x64

Repro steps.

Was this an app that wasn't created using the CLI? What change did you do on your code? etc.

My project is stable right now in beta.17 trying to use beta.18 or beta.19-3 on npm.

The log given by the failure.

Normally this include a stack trace and some more information.
No building error. It happens on runtime.

Mention any other details that might be useful.

Webpack seems to wrongly resolve indirect dependencies. To be more precise:
0) Grab a ng new project

  1. Install a package X with dependency Y (let's call it oldY).
  2. install package Y (lets call it newY).
  3. On ng serve (looking at the main bundle if you wish), package X will receive newY instead.

I personally did these tests for packages X = jsonapi-serializer, Y = lodash (or Y = bluebird)

This error seems to be related to #2291. Commenting out the line changed in this PR makes webpack resolve correctly as before.


Thanks! We'll be in touch soon.

@clydin
Copy link
Member

clydin commented Nov 4, 2016

PR #2291 disables node's module resolution algorithm. I had to change it locally as well.

@filipesilva filipesilva added command: build P1 Impacts a large percentage of users; if a workaround exists it is partial or overly painful needs: investigation Requires some digging to determine if action is needed labels Nov 5, 2016
@clydin
Copy link
Member

clydin commented Nov 6, 2016

@filipesilva, So what's happening is that PR #2291 forces webpack to only look for packages in the first level of the projects local node_modules. This means that packages with the same dependency but with varying version needs, or npm linked packages with unique dependencies (i.e., not found in the project) will fail.

I dug through webpack's dependency resolution code after #2291 was merged and it looks like my suggested change from that PR is the proper way to define a backup module resolution strategy.
Upon further investigation, my theory is that awesome-typescript-loader's custom dependency graph analysis was the cause of that change to fail. TS 2.0+ uses the realpath of a symlink to resolve dependencies which was then causing webpack to perform the node module resolution algorithm on the wrong directory hierarchy. (microsoft/TypeScript#9552)
Unfortunately, I haven't had time to experiment with any code changes yet.

@filipesilva
Copy link
Contributor

@clydin thank you for the analysis and solution. Would you be available to do a PR with your solution?

@clydin
Copy link
Member

clydin commented Nov 16, 2016

So there are two parts. If just the CLI's webpack module resolution is changed, it would fix this issue but it would break npm linked angular libraries again (AOT mode should work though). The current typescript loader would need changes as well for a complete fix. I have some experiments done for this but nothing concrete. (It also looks like there is some major refactoring taking place for the next version). Also, ts-loader handles dependency resolution differently than ATL, so that could potentially be an option as well.

@filipesilva
Copy link
Contributor

We might be looking at making the AoT loader our default one in the future, so that information is definitely useful. Thank you.

@ShadowManu
Copy link
Author

Any news or something we can help with? Our team is halted at beta.17 (not painful yet) since the bug will break the dependencies.

@hansl
Copy link
Contributor

hansl commented Feb 3, 2017

Closing this as AOT compatible libraries are needed now. Most libraries that we contacted already published AOT-compatible versions on npm.

@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 6, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
needs: investigation Requires some digging to determine if action is needed P1 Impacts a large percentage of users; if a workaround exists it is partial or overly painful
Projects
None yet
Development

No branches or pull requests

4 participants