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 relative paths with bundleDependencies=false #18488

Merged
merged 1 commit into from Aug 10, 2020

Conversation

amakhrov
Copy link
Contributor

@amakhrov amakhrov commented Aug 8, 2020

When bundleDependencies is turned off, webpack is configured to bundle all modules imported via relative path.
It works well for import paths like './some/file' but fails for imports traversing the dir structure up, like '../some/file'.

In most cases it would still work well, because of the additional fallback (which checks whether node can resolve this module at all). However, in rare cases this fallback fails (see https://github.com/angular/universal/issues/1792)

Closes: #18493

@googlebot

This comment has been minimized.

@amakhrov
Copy link
Contributor Author

amakhrov commented Aug 8, 2020

@googlebot I signed it!

@googlebot
Copy link

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

@googlebot googlebot added cla: yes and removed cla: no labels Aug 8, 2020
Copy link
Collaborator

@alan-agius4 alan-agius4 left a comment

Choose a reason for hiding this comment

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

The commit message is not valid.

The commit message header should start with fix(@angular-devkit/build-angular):

See: https://github.com/angular/angular-cli/blob/master/CONTRIBUTING.md

@amakhrov amakhrov force-pushed the fix-relative-path-detection branch 2 times, most recently from 98ac3b0 to 5b7f32a Compare August 9, 2020 08:39
… bundleDependencies=false

When `bundleDependencies` is turned off, webpack only bundles modules imported via relative path.
Existing check works well for import paths like `'./some/file'`,
but fails for imports traversing the dir structure up, like `'../some/file'`.
@amakhrov amakhrov force-pushed the fix-relative-path-detection branch from 5b7f32a to ac6611c Compare August 9, 2020 17:01
Copy link
Collaborator

@alan-agius4 alan-agius4 left a comment

Choose a reason for hiding this comment

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

Thanks LGTM

@alan-agius4 alan-agius4 added action: merge The PR is ready for merge by the caretaker target: patch This PR is targeted for the next patch release labels Aug 9, 2020
@amakhrov
Copy link
Contributor Author

amakhrov commented Aug 9, 2020

Thanks for the super quick feedback @alan-agius4 - quite impressive!

@amakhrov
Copy link
Contributor Author

amakhrov commented Aug 9, 2020

@alan-agius4 I have a followup question though.
While the PR seems to fix the specific issue I was facing, I'm wondering if there are more cases that could fail like that.

As I mentioned, despite the incomplete original check for partial path, the builds tend to work well.
The reason is that if this check returns false, the code below has an extra check - tries to resolve the path via require.resolve. But this resolve call is performed in the context of server.ts file, not in the context of the compiled module.
Hence, in my case the "../../../utils" path was successfully resolved! Because it matches utils from angular_devkit itself

 /Users/amakhrov/Projects/angular-testing/node_modules/@angular-devkit/build-angular/src/utils/index.js

Do you think it's possible that this check can potentially give false positives for some arbitrary user module alias, if it also happens to match some internal module from devkit?

@alan-agius4
Copy link
Collaborator

@amakhrov, glad you hear that you had a good experience contributing to the Angular CLI. 😁

I do think that there are some rare edge cases, where using require.resolve(request) might cause undesired behavior. A more correct way would be to changing the resolutions paths to use the workspace root path. require.resolve(request, { paths: [wco.root] }).

@filipesilva filipesilva merged commit 042c33c into angular:master Aug 10, 2020
@amakhrov amakhrov deleted the fix-relative-path-detection branch August 10, 2020 15:57
@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 10, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: merge The PR is ready for merge by the caretaker target: patch This PR is targeted for the next patch release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cannot find module '../../../utils' when bundleDependencies=false
4 participants