Add missing source-map methods #2

onigoetz · 2022-06-16T20:39:36Z

Hi,

As said in #1 This PR adds some missing methods to source-map, I'm opening it as Draft as it's not yet complete but I would love some feedback on a few things.

Added Methods and fields

static SourceMapConsumer.fromSourceMap() will transform a SourceMapGenerator into a SourceMapConsumer. is optimized to use toDecodedMap when available
SourceMapConsumer.version
SourceMapConsumer.mappings: computes the mappings on demand
SourceMapConsumer.generatedPositionFor()
SourceMapConsumer.allGeneratedPositionsFor(): Will only return one value from generatedPositionFor
SourceMapConsumer.hasContentsOfAllSources()
SourceMapConsumer.sourceContentFor()
- Exposed the function from trace-mapping
- but source-map-js does a resolution of the path as well: https://github.com/7rulnik/source-map-js/blob/patch-0.6.1/lib/source-map-consumer.js#L750-L794, can be done later as a PR to trace-mapping directly
SourceMapConsumer.eachMapping()
- Doesn't support the "order" argument yet
static SourceMapGenerator.fromSourceMap()
~~SourceMapGenerator.applySourceMap~~
- Removed from the PR as waiting on Add applySourceMap gen-mapping#3
src/utils: needed

Tests

Tests come from the source-map-js repository, I've been testing these changes on a PR I'm preparing for postcss I will probably add more tests and examples as I discover and fix issues.

Tests Runtime

I configured the tests to use ts-mocha instead of mocha and changed the paths in the tests to use the source file, as it didn't show the coverage properly. This PR currently has 85% coverage.

---------------|---------|----------|---------|---------|
File           | % Stmts | % Branch | % Funcs | % Lines |              
---------------|---------|----------|---------|---------|
All files      |   85.32 |    78.08 |   88.46 |   85.32 |                                           
 source-map.ts |   89.95 |    90.32 |      85 |   89.95 | 
 util.ts       |   80.64 |    69.04 |     100 |   80.64 |
---------------|---------|----------|---------|---------|

Build

This PR also introduces changes in the build process. It now has three outputs instead of two :

source-map.umd.js : For browsers, still same as before
source-map.mjs.js: For node with imports, does not include dependencies
source-map.cjs: For node with requires, does not include dependencies

I also changed the package.json accordingly.

If you have any early feedback before I go in a wrong direction I'd love to hear it

rollup.config.js

src/source-map.ts

onigoetz · 2022-06-16T20:43:38Z

src/source-map.ts

+  allGeneratedPositionsFor(
+    originalPosition: Parameters<typeof generatedPositionFor>[1],
+  ): ReturnType<typeof generatedPositionFor>[] {
+    // This doesn't map exactly to the same feature
+    return [generatedPositionFor(this._map, originalPosition)];
+  }


It seems there is no equivalent in trace-mapping I'm not sure to know how important it is or not

Not yet, but the instanbul project might want to use it eventually to fix their coverage cases.

Documented for now that it only returns one mapping

onigoetz · 2022-06-16T20:44:35Z

src/source-map.ts

+    const sourceContent = sourceContentFor(this._map, aSource);
+    if (sourceContent != null) {
+      return sourceContent;
+    }
+
+    const resolvedSource = resolve(aSource, this.sourceRoot);
+    if (this.sources.indexOf(resolvedSource) > -1) {
+      return this.sourcesContent[this.sources.indexOf(resolvedSource)];
+    }


should this logic be moved to trace-mapping ?

It's already there. 😉

Only partially though.

the implementation in trace-mapping looks up a static list of sources, whereas the original Mozilla implementation resolves the sourceFile and accepts some relative paths.

Removed the extended part for now and will look into a future PR for trace-mapping

onigoetz · 2022-06-16T20:45:18Z

src/source-map.ts

+
+  eachMapping(
+    aCallback: Parameters<typeof eachMapping>[1],
+    aContext?: any /*, aOrder?: number*/,


I don't know how important that order argument is, should it be implemented ? is it even possible in trace-mapping ?

This is possible in trace-mapping, but it's not implemented. We generate the "original order" in generatedPositionFor, and you would need to walk that. Rather than adding a parameter to trace-mapping, I would expose a new function that did eachMappingBySource, and call the appropriate function here.

Documented that this isn't supported for now

src/source-map.ts

rollup.config.js

src/source-map.ts

jridgewell · 2022-06-18T21:08:26Z

src/source-map.ts

+  allGeneratedPositionsFor(
+    originalPosition: Parameters<typeof generatedPositionFor>[1],
+  ): ReturnType<typeof generatedPositionFor>[] {
+    // This doesn't map exactly to the same feature
+    return [generatedPositionFor(this._map, originalPosition)];
+  }


Not yet, but the instanbul project might want to use it eventually to fix their coverage cases.

src/source-map.ts

jridgewell · 2022-06-18T21:31:30Z

src/source-map.ts

-    this._map = new GenMapping(opts);
+  constructor(opts: ConstructorParameters<typeof GenMapping>[0] | GenMapping) {
+    // TODO :: should this be duck-typed ?
+    this._map = opts instanceof GenMapping ? opts : new GenMapping(opts);


Does SourceMapGenerator support this?

It doesn't but I use this as a workaround to make it possible to call the constructor in the fromSourceMap below that is using fromMap() and passing that to SourceMapGenerator's constructor

jridgewell · 2022-06-18T21:33:59Z

src/source-map.ts

+    }
+
+    // This is a fallback for `source-map` and `source-map-js`
+    return new SourceMapConsumer(map.toJSON() as SectionedSourceMapInput, mapUrl);


Do we need to support the old consumer instances? I'd think most would be from the JSON string/Object forms that are serialized into a file.

I think we do, I honestly don't know in which contexts this package is used and sometimes compilation processes go through multiple libraries which could use any source map library

src/source-map.ts

TylorS · 2022-06-25T17:32:57Z

Hello @onigoetz and @jridgewell! Just came across these packages and this PR and just wanted to thank you both for doing amazing work to dramatically improve the codebase and size of source-map!

It's a lot harder to justify the bundle size of source-map in the browser for small features like enhancing error messages as I'm keen to do, and this seems incredibly promising. Please let me know if I can be of any help

jridgewell · 2022-06-27T04:37:34Z

@TylorS from you're comment, I imagine you're doing unminification of stack trace line/columns using the sourcemap? I'd suggest using @jridgewell/trace-mapping directly instead of this package, so that unused code can be eliminated from your bundle. That won't be fully possible with this package, because using the SourceMapConsumer class will hold all of trace-mapping's functions regardless of whether you actually use them.

TylorS · 2022-06-27T15:34:51Z

Thanks for the advice @jridgewell! I'll have to poke around at it.

I was initially going to use github.com/xpl/stacktracey, but it has a direct dep on source-map, and I was considering forking it to have a dependency on this library instead

onigoetz · 2023-01-17T21:25:50Z

Hi @jridgewell

I've been fairly inactive on my JS projects lately but would like to pick up where I left off.
How can I help to get this PR moving ?

I'm still very motivated to get this PR merged and get more tools over to your implementation of source-map to make JS tooling faster.

onigoetz · 2024-03-04T20:25:37Z

Hi @jridgewell

I think I addressed all comments from the PR except the ones that I left unresolved.
I removed the applySourceMaps implementation for now and some methods are not 1:1 equivalents to source-map or source-map-js yet. I documented that as well.

I'm ready to make adjustments wherever is needed

jridgewell · 2024-03-06T02:11:37Z

I'll take a look at this this weekend.

onigoetz commented Jun 16, 2022

View reviewed changes

rollup.config.js Show resolved Hide resolved

onigoetz commented Jun 16, 2022

View reviewed changes

src/source-map.ts Outdated Show resolved Hide resolved

onigoetz commented Jun 16, 2022

View reviewed changes

src/source-map.ts Outdated Show resolved Hide resolved

onigoetz commented Jun 16, 2022

View reviewed changes

src/source-map.ts Outdated Show resolved Hide resolved

jridgewell reviewed Jun 18, 2022

View reviewed changes

onigoetz mentioned this pull request Jun 21, 2022

Add applySourceMap jridgewell/gen-mapping#3

Open

jridgewell mentioned this pull request Aug 9, 2022

Is this library "terser internal"? #3

Closed

onigoetz and others added 3 commits March 4, 2024 19:25

Add missing source-map methods

1a8b1ad

Remove changes that are done by the PR to resolve-uri

cc2da5c

Remove applySourceMap for now

dfcccc7

onigoetz force-pushed the main branch from 8a2cf59 to dfcccc7 Compare March 4, 2024 18:30

onigoetz added 4 commits March 4, 2024 19:56

simplify sourceContentFor

d0915a1

Improve hasContentsOfAllSources and add tests

ec29a58

Add new methods to readme

30c81f6

Polish readme

7351da6

onigoetz marked this pull request as ready for review March 4, 2024 20:20

jridgewell merged commit 410f3e3 into jridgewell:main Mar 12, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add missing source-map methods #2

Add missing source-map methods #2

onigoetz commented Jun 16, 2022 •

edited

onigoetz Jun 16, 2022

jridgewell Jun 18, 2022

onigoetz Mar 4, 2024

onigoetz Jun 16, 2022

jridgewell Jun 18, 2022

onigoetz Mar 4, 2024

onigoetz Jun 16, 2022

jridgewell Jun 18, 2022

onigoetz Mar 4, 2024

jridgewell Jun 18, 2022

jridgewell Jun 18, 2022

onigoetz Jun 19, 2022

jridgewell Jun 18, 2022

onigoetz Mar 4, 2024

TylorS commented Jun 25, 2022

jridgewell commented Jun 27, 2022

TylorS commented Jun 27, 2022

onigoetz commented Jan 17, 2023

onigoetz commented Mar 4, 2024

jridgewell commented Mar 6, 2024

Add missing source-map methods #2

Add missing source-map methods #2

Conversation

onigoetz commented Jun 16, 2022 • edited

Added Methods and fields

Tests

Tests Runtime

Build

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TylorS commented Jun 25, 2022

jridgewell commented Jun 27, 2022

TylorS commented Jun 27, 2022

onigoetz commented Jan 17, 2023

onigoetz commented Mar 4, 2024

jridgewell commented Mar 6, 2024

onigoetz commented Jun 16, 2022 •

edited