Babel ast location, diagnostic, and source location remapping #6238
Conversation
# Conflicts: # packages/core/core/package.json # packages/core/core/src/Transformation.js # packages/core/utils/package.json # packages/optimizers/cssnano/package.json # packages/optimizers/esbuild/package.json # packages/optimizers/terser/package.json # packages/packagers/css/package.json # packages/packagers/js/package.json # packages/shared/babel-ast-utils/package.json # packages/shared/scope-hoisting/package.json # packages/transformers/coffeescript/package.json # packages/transformers/css/package.json # packages/transformers/less/package.json # packages/transformers/sass/package.json # packages/transformers/typescript-types/package.json # packages/transformers/vue/package.json
|
This pull request has been linked to and will mark 1 task as "Done" when merged:
|
| let assetGroup = { | ||
| code, | ||
| mapBuffer: sourcemap.toBuffer(), |
There was a problem hiding this comment.
This is still going to be an issue as it (along with the code) gets serialized into the bundlegraph. cc @DeMoorJasper @devongovett
There was a problem hiding this comment.
Yes. Do we actually need the sourcemap here? It's empty and only contains the same code as is already passed through the request. If there's no existing map, won't it already be set as the source content during transformation? I guess we should have an integration test for runtimes + sourcemaps.
There was a problem hiding this comment.
Another issue is that with empty maps all of the columns mappings are zero when the empty map is extended. If we overwrote the filePath here though it breaks a lot of tests that expect JSRuntime.js as the name. Maybe the test util should just ignore runtime assets?
wbinnssmith
force-pushed
the
wbinnssmith/swc-babel-remap-asts
branch
from
102213a
to
e0abdbc
Compare
wbinnssmith
force-pushed
the
wbinnssmith/swc-babel-remap-asts
branch
from
e0abdbc
to
6aeb81a
Compare
Benchmark ResultsKitchen Sink 🚨
Timings
Cold BundlesNo bundles found, this is probably a failed build... Cached BundlesNo bundles found, this is probably a failed build... React HackerNews 🚨
Timings
Cold BundlesNo bundles found, this is probably a failed build... Cached BundlesNo bundles found, this is probably a failed build... AtlasKit Editor 🚨
Timings
Cold BundlesNo bundles found, this is probably a failed build... Cached BundlesNo bundles found, this is probably a failed build... Three.js 🚨
Timings
Cold BundlesNo bundles found, this is probably a failed build... Cached BundlesNo bundles found, this is probably a failed build... |
| if (sourceMap) { | ||
| let promises = []; | ||
| // Traverse the bundle to get all source contents | ||
| this.bundle.traverseAssets(asset => { |
There was a problem hiding this comment.
This doesn't seem quite right. Not all of the assets in the bundle will necessarily be included in the final output. Some might be skipped e.g. due to no used symbols. Also, shouldn't the sourcemap library handle this when merging the source maps from the original asset?
There was a problem hiding this comment.
I think you're right the new scope hoisting no longer needs this as now you're using the sourcemap library already and not relying on the AST for all of this.
Previously we were relying on the Babel AST for this and the AST does not keep a reference to the source content so we had to extract that from the maps and manually add this. We can probably refactor this a bit further to make it a bit more lightweight now that the scope-hoisting implementation already supports the source-maps properly.
| if (existing == null) { | ||
| // If no existing sourcemap was found, initialize sourcesContent with | ||
| // the asset's filepath and its original contents. | ||
| asset.sourcesContent = { |
There was a problem hiding this comment.
Why is this stored separately on the asset rather than in the source map? If there's no existing source map, should we just initialize an empty one containing only the original code?
There was a problem hiding this comment.
…h/swc-babel-remap-asts # Conflicts: # packages/transformers/babel/src/babel7.js
…h/swc-babel-remap-asts
| @@ -24,6 +25,42 @@ import {generator, expressionsPrecedence} from './generator'; | |||
|
|
|||
| export {babelErrorEnhancer}; | |||
|
|
|||
| export function remapAstLocations(ast: BabelNodeFile, map: SourceMap) { | |||
| // remap ast to original mappings | |||
| // This improves sourcemap accuracy and fixes sourcemaps when scope-hoisting | |||
There was a problem hiding this comment.
I guess this is only used if you are using babel and there's an existing sourcemap now?
|
Changed a few things in #6286. LMK if I missed something. |
* Simplify source content handling * Copy over source content from original map in babel transformer * Don't store a source map in the graph * Remap diagnostics and other source locations using input source map (#6288) * Remap diagnostics and other source locations using input source map * Fix flow * Ignore test in flow config * Move to util, apply in CSS transformer as well
devongovett
changed the title
…raph * bdo/buffer-backed-graph: (37 commits) Improve resolver performance (#6328) v2.0.0-beta.3.1 Add it to the nightly release workflow as well Configure jemalloc page size for M1 (#6314) Update swc (#6307) Fix parcelDependencies v2.0.0-beta.3 Update swc (#6289) Remove old AdjacencyList, serialize EfficientGraph in Graph, update BundleGraph to use new functions Babel ast location, diagnostic, and source location remapping (#6238) update contributing guide (#6293) Update sourcemap to rc-1.0 (#6279) Serve nearest index.html in case the applications has multiple index files (#6250) Allow bundling browserify bundles by replacing free requires with undefined (#6260) Upgrade flow to 0.151.0 (#6281) Use local require for `@babel/core` and `postcss` (#6264) Resolve helpers relative to JS transformer (#6278) Pure comment for $parcel$interopDefault (#6271) Don't transpile spread in JSX with modern targets (#6274) Update TS validator assertions (#6272) ...
#5871 applied to #6230.
This also adds a fallback to read and use the original asset's code as sourceContent when an existing sourcemap is not found.
Fixes T-1042
Test Plan:
This should be implemented as an integration test.Added an integration test