fix(ivy): ngcc - resolve main property paths correctly
#31509
Closed
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
petebacondarwin
added
type: bug/fix
action: review
petebacondarwin
force-pushed
the
ngcc-isumdmodule-bug-FW-1444
branch
3 times, most recently
from
a331755
to
0ab9f93
Compare
petebacondarwin
added
action: merge
kapunahelewong
pushed a commit
to kapunahelewong/angular
that referenced
this pull request
Jul 11, 2019
…ular#31509) This reverts commit 103a5b4.
gkalpak
approved these changes
Jul 11, 2019
gkalpak
added
action: cleanup
Agh! Yes, all of the |
|
I assumed that for most fields (such as |
|
I don't really know but it seems to make sense to cover our backs. |
petebacondarwin
force-pushed
the
ngcc-isumdmodule-bug-FW-1444
branch
from
0ab9f93
to
e703a3c
Compare
|
@gkalpak I cleaned it up and moved stuff around. Might be worth another look from you. |
petebacondarwin
force-pushed
the
ngcc-isumdmodule-bug-FW-1444
branch
from
e703a3c
to
965648e
Compare
gkalpak
approved these changes
Jul 11, 2019
packages/compiler-cli/ngcc/test/dependencies/umd_dependency_host_spec.ts
Outdated
Show resolved
Hide resolved
There are two places in the ngcc processing where it needs to load the content of a file given by a general path: * when determining the format of an entry-point. To do this ngcc uses the value of the relevant property in package.json. But in the case of `main` it must parse the contents of the entry-point file to decide whether the format is UMD or CommonJS. * when parsing the source files for dependencies to determine the order in which compilation must occur. The relative imports in each file are parsed and followed recursively, looking for external imports. Previously, we naively assumed that the path would match the file name exactly. But actually we must consider the standard module resolution conventions. E.g. the extension (.js) may be missing, or the path may refer to a directory containing an index.js file. This commit fixes both places. This commit now requires the `DependencyHost` instances to check the existence of more files than before (at worst all the different possible post-fixes). This should not create a significant performance reduction for ngcc. Since the results of the checks will be cached, and similar work is done inside the TS compiler, so what we lose in doing it here, is saved later in the processing. The main performance loss would be where there are lots of files that need to be parsed for dependencies that do not end up being processed by TS. But compared to the main ngcc processing this dependency parsing is a small proportion of the work done and so should not impact much on the overall performance of ngcc. // FW-1444
petebacondarwin
force-pushed
the
ngcc-isumdmodule-bug-FW-1444
branch
from
965648e
to
e7ba641
Compare
petebacondarwin
added
action: merge
sabeersulaiman
pushed a commit
to sabeersulaiman/angular
that referenced
this pull request
Sep 6, 2019
There are two places in the ngcc processing where it needs to load the content of a file given by a general path: * when determining the format of an entry-point. To do this ngcc uses the value of the relevant property in package.json. But in the case of `main` it must parse the contents of the entry-point file to decide whether the format is UMD or CommonJS. * when parsing the source files for dependencies to determine the order in which compilation must occur. The relative imports in each file are parsed and followed recursively, looking for external imports. Previously, we naively assumed that the path would match the file name exactly. But actually we must consider the standard module resolution conventions. E.g. the extension (.js) may be missing, or the path may refer to a directory containing an index.js file. This commit fixes both places. This commit now requires the `DependencyHost` instances to check the existence of more files than before (at worst all the different possible post-fixes). This should not create a significant performance reduction for ngcc. Since the results of the checks will be cached, and similar work is done inside the TS compiler, so what we lose in doing it here, is saved later in the processing. The main performance loss would be where there are lots of files that need to be parsed for dependencies that do not end up being processed by TS. But compared to the main ngcc processing this dependency parsing is a small proportion of the work done and so should not impact much on the overall performance of ngcc. // FW-1444 PR Close angular#31509
|
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
When determining if a
mainpath points to a UMD or CommonJSformat, the contents of the file need to be loaded and parsed.
Previously, it was assumed that the path referred to the exact filename,
but did not account for normal module resolution semantics, where the
path may be missing an extension or refer to a directory containing an
index.jsfile.// FW-1444
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
Issue Number: N/A
What is the new behavior?
Does this PR introduce a breaking change?
Other information