Linker external source map preemptive #40237
Conversation
12f0588
to
46bd652
Some typos in these commits:
refactor(compiler-cli): add file to Babel locations
The filename of the source-span is now added to the Babel location
when setting the source-map range in the `BabelAstHost`.
Note that the filename is only added if it is different to the main file
being processed. Otherwise Babel will generate two entries ___in___ its
generated source-map.
refactor(compiler-cli): support external template source-mapping when…
… linking
This commit changes the ___`___PartialComponentLinker` to use the original source
of an external template when compiling, if available, to ensure that the
source-mapping of the final linked code is accurate.
If the linker is given a file-system and logger, then it will attempt
to compute the original source of external templates so that the final
linked code references the correct template source.
|
|
||
| for (const mapping of this.flattenedMappings) { | ||
| const sourceIndex = findIndexOrAdd(sources, mapping.originalSource); | ||
| const sourceIndex = sources.set( | ||
| this.fs.relative(sourcePathDir, mapping.originalSource.sourcePath), |
I have observed this path manipulation to add noticeable overhead in TypeScript (microsoft/TypeScript#40130) and source-map (mozilla/source-map#308) so we may want to cache this computation somehow.
I think a cache for each call to renderFlattenedSourceMap() should be adequate. Thanks for spotting that.
packages/compiler-cli/linker/babel/test/ast/babel_ast_factory_spec.ts
Outdated
Show resolved
Hide resolved
| return null; | ||
| } | ||
|
|
||
| const sourceFileLoader = new SourceFileLoader(this.options.fileSystem, this.options.logger, {}); |
Should schemeMap really be empty here?
Good question. This is only here to get around issues with webpack creating synthesized paths to files (e.g. webpack://blah/blah/blah.
In the i18n extractor we fix this by adding {webpack: basePath}. But I note that we leave this empty in ngcc.
I think we should leave it out for the time-being, and fix it up if we find that "in the wild" we are getting similar problems with synthesized paths... unless you are confident that we will indeed hit this problem right now? I suspect in any case, we would hit it (or not) before we get to rolling this out in full production...
packages/compiler-cli/linker/src/file_linker/partial_linkers/partial_component_linker_1.ts
Outdated
Show resolved
Hide resolved
| return null; | ||
| } | ||
|
|
||
| const templateContents = sourceFile.sources.find(src => src?.sourcePath === pos.file)!.contents; |
Is the non-null assertion really safe here? I would err on the side of caution and safely return null in favor of the non-null assertion.
Yes it really is safe, since the pos is returned from getOriginalSourceLocation() which guarantees that if the pos !== null then pos.file must be the path to one of the sources. See
packages/compiler-cli/linker/src/file_linker/partial_linkers/partial_component_linker_1.ts
Outdated
Show resolved
Hide resolved
packages/compiler-cli/linker/src/file_linker/partial_linkers/partial_component_linker_1.ts
Show resolved
Hide resolved
46bd652
to
31e6520
|
I see that you just added the If you want your PR to be merged, it has to pass all the CI checks. If you can't get the PR to a green state due to flakes or broken master, please try rebasing to master and/or restarting the CI job. If that fails and you believe that the issue is not due to your change, please contact the caretaker and ask for help. |
2b80b8e
to
b8fc5ef
…pilation When partially compiling a component with an external template, we must synthesize a new AST node for the string literal that holds the contents of the external template, since we want to source-map this expression directly back to the original external template file.
… source-maps When a source-map/source-file tree has nodes that refer to the same file, the flattened source-map rendering was those files multiple times, rather than consolidating them into a single source-map source.
The filename of the source-span is now added to the Babel location when setting the source-map range in the `BabelAstHost`. Note that the filename is only added if it is different to the main file being processed. Otherwise Babel will generate two entries in its generated source-map.
These imports were unnecessrily deep, since the files are actually in the same directory.
Previously the names of the source and expectation files were often reused, which caused potential confusion. There is now a single source file for each test-case, which is important when they are being compiled with different compiler options, since the GOLDEN_PARTIAL file will only contain one copy per file name. The names of the expectation files have now been changed so that is clearer which test-case they are related to.
…ssages Now, if a source-mapping compliance test fails, the message displays both the path to the generated file, and more helpfully the path to the expected file.
This commit migrates, and supplements, compliance tests that check the source-mapping of external templates.
… linking This commit changes the `PartialComponentLinker` to use the original source of an external template when compiling, if available, to ensure that the source-mapping of the final linked code is accurate. If the linker is given a file-system and logger, then it will attempt to compute the original source of external templates so that the final linked code references the correct template source.
b8fc5ef
to
b9afb56
The filename of the source-span is now added to the Babel location when setting the source-map range in the `BabelAstHost`. Note that the filename is only added if it is different to the main file being processed. Otherwise Babel will generate two entries in its generated source-map. PR Close #40237
These imports were unnecessrily deep, since the files are actually in the same directory. PR Close #40237
#40237) Previously the names of the source and expectation files were often reused, which caused potential confusion. There is now a single source file for each test-case, which is important when they are being compiled with different compiler options, since the GOLDEN_PARTIAL file will only contain one copy per file name. The names of the expectation files have now been changed so that is clearer which test-case they are related to. PR Close #40237
This commit migrates, and supplements, compliance tests that check the source-mapping of external templates. PR Close #40237
… linking (#40237) This commit changes the `PartialComponentLinker` to use the original source of an external template when compiling, if available, to ensure that the source-mapping of the final linked code is accurate. If the linker is given a file-system and logger, then it will attempt to compute the original source of external templates so that the final linked code references the correct template source. PR Close #40237
|
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. |
No description provided.
The text was updated successfully, but these errors were encountered: