Skip to content

fix(compiler-cli): shorten resolved module name in fileNameToModuleName to npm package name for typings #23231

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

fix(compiler-cli): shorten resolved module name in fileNameToModuleName to npm package name for typings #23231

wants to merge 1 commit into from

Conversation

gregmagolan
Copy link
Contributor

@gregmagolan gregmagolan commented Apr 6, 2018 •
edited
Loading

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

[x] Bugfix
[ ] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Documentation content changes
[ ] angular.io application / infrastructure changes
[ ] Other... Please describe:

What is the current behavior?

Currently fileNameToModuleName resolves typings files to module names as such: .../node_modules/@angular/core/core.d.ts => @angular/core/core, .../node_modules/rxjs/index.d.ts => rxjs/index.

Issue Number: N/A

What is the new behavior?

If the typings in the npm package matches the imported file then the module name is resolved to the npm package name as such: .../node_modules/@angular/core/core.d.ts => @angular/core, .../node_modules/rxjs/index.d.ts => rxjs.

Does this PR introduce a breaking change?

[ ] Yes
[x] No

Other information

@gregmagolan gregmagolan requested a review from alexeagle April 6, 2018 18:12
@mary-poppins
Copy link

You can preview 12764a9 at https://pr23231-12764a9.ngbuilds.io/.

@alexeagle alexeagle requested a review from alxhub April 6, 2018 18:57
@gregmagolan gregmagolan force-pushed the shorten-module-name-for-typings branch from 12764a9 to 826fe9e Compare April 6, 2018 19:54
alexeagle
alexeagle reviewed Apr 6, 2018
View reviewed changes
integration/bazel/BUILD.bazel Outdated
# e.g. node_modules/xpath/docs/function resolvers.md
"node_modules/**/docs/**",
],
srcs = glob(["/".join([
Copy link
Contributor

Choose a reason for hiding this comment

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

let's keep it the other way (will be needed for hermeticity) and add node_modules/rxjs/** to the excludes with a comment explaining why we need to test for this case

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

packages/compiler-cli/src/transformers/compiler_host.ts Outdated
// if the npm package typings matches the importedFile
try {
const modulePath = importedFile.substring(0, importedFile.length - moduleName.length) + importedFilePackageName;
const packageJson = require(modulePath + '/package.json');
Copy link
Contributor

Choose a reason for hiding this comment

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

this probably gets called a lot, I think it's probably worth memoizing this lookup for performance

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

@mary-poppins
Copy link

You can preview 826fe9e at https://pr23231-826fe9e.ngbuilds.io/.

@gregmagolan gregmagolan force-pushed the shorten-module-name-for-typings branch from 826fe9e to 4619f11 Compare April 6, 2018 20:22
@mary-poppins
Copy link

You can preview 4619f11 at https://pr23231-4619f11.ngbuilds.io/.

@gregmagolan gregmagolan force-pushed the shorten-module-name-for-typings branch from 4619f11 to 912825a Compare April 6, 2018 20:43
@mary-poppins
Copy link

You can preview 912825a at https://pr23231-912825a.ngbuilds.io/.

@gregmagolan gregmagolan force-pushed the shorten-module-name-for-typings branch 2 times, most recently from eef9568 to baf0c91 Compare April 6, 2018 21:01
@mary-poppins
Copy link

You can preview eef9568 at https://pr23231-eef9568.ngbuilds.io/.

@mary-poppins
Copy link

You can preview baf0c91 at https://pr23231-baf0c91.ngbuilds.io/.

@alexeagle alexeagle added the target: major This PR is targeted for the next major release label Apr 11, 2018
alxhub
alxhub approved these changes Apr 12, 2018
View reviewed changes
packages/compiler-cli/src/transformers/compiler_host.ts
if (packageTypings === originalImportedFile) {
moduleName = importedFilePackageName;
}
} catch (e) {
Copy link
Member

Choose a reason for hiding this comment

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

Leave a comment explaining why ignoring the error is okay.

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

@gregmagolan gregmagolan force-pushed the shorten-module-name-for-typings branch from baf0c91 to 1954ad1 Compare April 12, 2018 03:09
@mary-poppins
Copy link

You can preview 1954ad1 at https://pr23231-1954ad1.ngbuilds.io/.

@alexeagle alexeagle added the action: merge The PR is ready for merge by the caretaker label Apr 12, 2018
@IgorMinar IgorMinar closed this in 6199ea5 Apr 13, 2018
wKoza pushed a commit to wKoza/angular that referenced this pull request Apr 13, 2018
@gregmagolan @wKoza
fix(compiler-cli): shorten resolved module name in fileNameToModuleNa…
1c02588 
…me to npm package name for typings (angular#23231)

PR Close angular#23231
wKoza pushed a commit to wKoza/angular that referenced this pull request Apr 13, 2018
@gregmagolan @wKoza
fix(compiler-cli): shorten resolved module name in fileNameToModuleNa…
8112925 
…me to npm package name for typings (angular#23231)

PR Close angular#23231
wKoza pushed a commit to wKoza/angular that referenced this pull request Apr 13, 2018
@gregmagolan @wKoza
fix(compiler-cli): shorten resolved module name in fileNameToModuleNa…
65d9c40 
…me to npm package name for typings (angular#23231)

PR Close angular#23231
@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 13, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@alexeagle alexeagle alexeagle approved these changes

@alxhub alxhub alxhub approved these changes

Assignees
No one assigned
Labels
action: merge The PR is ready for merge by the caretaker cla: yes target: major This PR is targeted for the next major release
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@gregmagolan @mary-poppins @alexeagle @alxhub @googlebot