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

feat: migrate from source-map to @jridgewell/trace-mapping #12692

Merged
merged 3 commits into from
Apr 20, 2022

Conversation

SimenB
Copy link
Member

@SimenB SimenB commented Apr 19, 2022

Summary

Same motivation as istanbuljs/v8-to-istanbul#185.

@jridgewell hi again! 😀 If this PR lands, there's only a single (non-transitive) usage of source-map in this repo, and that's https://github.com/facebook/jest/blob/5183c15a8e14089351d6403f4fdb72a6705aa379/e2e/coverage-handlebars/transform-handlebars.js. Would that use case be something you support as well?

Test plan

Green CI

@@ -456,7 +450,7 @@ export default class CoverageReporter extends BaseReporter {
originalSource: fileTransform.originalCode,
source: fileTransform.code,
sourceMap: {
sourcemap: {file: res.url, ...sourcemapContent},
sourcemap: {file: res.url, ...sourcemapContent} as any,
Copy link
Member Author

Choose a reason for hiding this comment

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

@@ -26,8 +26,8 @@ export interface Options
isInternalModule?: boolean;
}

// This is fixed in source-map@0.7.x, but we can't upgrade yet since it's async
interface FixedRawSourceMap extends Omit<RawSourceMap, 'version'> {
// `babel` and `@jridgewell/trace-mapping` disagrees
Copy link
Member Author

Choose a reason for hiding this comment

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

@jridgewell
Copy link
Contributor

jridgewell commented Apr 20, 2022

Would that use case be something you support as well?

trace-mapping is specific to consumer side of the sourcemap API. I've been thinking about making a package for the generator side (there are a ton of optimizations that could be made over SourceMapGenerator), but haven't done that yet.

However, you don't really need a generator for this simple usecase. You can just take advantage of a SectionedSourceMap:

import { AllMap, encodedMappings } from '@jridgewell/trace-map';

const output = [
  'const Handlebars = require("handlebars/dist/cjs/handlebars.runtime.js");\n',
  'module.exports = Handlebars.template(',
  code,
  ');\n',
];

const map = new AllMap({
  version: 3,
  sections: [
    {
      offset: {
        // 0-index, so count of newlines before `code`
        line: 1,
        // column offset of the injected code on its first line
        column: output[1].length,
      },
      map: pc.map,
    },
  ],
});

// map is now a regular, non-sectioned sourcemap that can be used everywhere.

// You need to manually JSONify the map from here, but I could add an export to do that for you.
const json = JSON.stringify({
  version: 3,
  file: map.file,
  names: map.names,
  sources: map.sources,
  sourcesContent: map.sourcesContent,
  // Depending on where this is going, you could use decodedMappings to avoid an encode->decode round trip.
  mappings: encodedMappings(map),
});

@SimenB
Copy link
Member Author

SimenB commented Apr 20, 2022

Thanks @jridgewell! keeping the current code in this case seems fine, it's just an integration test. Happy to switch over if/when you release a generator package, though! 🙂

@github-actions
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 21, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants