Questions about bundling and tests

[onigoetz]

Hi,

I'm working on a PR to this package to add some methods that are missing to this package compared to source-map and source-map-js

I'm curious about a few things in this package and wanted to ask before making a PR.

1. I see @jridgewell/gen-mapping and @jridgewell/trace-mapping are declared as dependencies but when bundled they aren't declared as externals. Should they be treated as externals (at least for non browser bundles) or should the dependencies be "dev" dependencies ? my 2 cent is that it should be externals.

2. Tests are run on the compiled code, using the source map to show the coverage and that's not working well. Is that on purpose or can I switch to use ts-mocha instead ?

[jridgewell]

Hello!

I'm working on a PR to this package to add some methods that are missing to this package

I should caution you that this package is meant to only be a simple wrapper around trace-mapping and gen-mapping, and that features should be added into those packages instead. Unless it's already implemented there, then we should just add the appropriate code to wrap it here.

1. I see @jridgewell/gen-mapping and @jridgewell/trace-mapping are declared as dependencies but when bundled they aren't declared as externals. Should they be treated as externals (at least for non browser bundles) or should the dependencies be "dev" dependencies ? my 2 cent is that it should be externals.

This is actually on purpose. terser needed something that can be linked to with only a single <script> tag, so in order to replace the old source-map package with this one, I had to bundle the deps into the output.

2. Tests are run on the compiled code, using the source map to show the coverage and that's not working well.

That might be because I didn't extensively test this package, it's just a few cursory tests to make sure it worked. If we added more tests, it's likely that the coverage would improve.

[onigoetz]

Thanks for the answers, I do have more questions :)

I should caution you that this package is meant to only be a simple wrapper around trace-mapping and gen-mapping

Agreed, and it's mostly what I'm doing. except for one method that's a bit more complex: applySourceMap which adds a dependency to @ampproject/remapping. I'm making this PR to propose this package to postcss which has source-maps part of its API, so It needs to be api-compliant.

This is actually on purpose. terser needed something that can be linked to with only a single <script> tag

I understand that for the browser build, but is it also the case for the esm build ? Could we add a node (commonjs) target as well to avoid bundled duplicates ?

Because now, the trace-mapping package contains a copy of @jridgewell/resolve-uri and @jridgewell/sourcemap-codec
And gen-mapping contains a copy of @jridgewell/set-array, @jridgewell/sourcemap-codec, and @jridgewell/trace-mapping which already contains @jridgewell/sourcemap-codec

This means that the package @jridgewell/source-map contains @jridgewell/trace-mapping twice, and @jridgewell/sourcemap-codec twice

If we added more tests, it's likely that the coverage would improve.

I'll have a look at the other packages you have that are tested this way, when enabling the lcov reporter to have an html output, I can't see the content of source-map.ts because ironically, the source-map points to a non-existing file.

I added more tests, and indeed the coverage improved, but I still wasn't able to use the HTML UI

[jridgewell]

applySourceMap which adds a dependency to @ampproject/remapping. I'm making this PR to propose this package to postcss which has source-maps part of its API, so It needs to be api-compliant.

Sounds good.

I understand that for the browser build, but is it also the case for the esm build ? Could we add a node (commonjs) target as well to avoid bundled duplicates ?

Sure. I think we can just do this for the browser build.

This means that the package @jridgewell/source-map contains @jridgewell/trace-mapping twice, and @jridgewell/sourcemap-codec twice

Sigh, I thought rollup would dedupe that during the build.

I'll have a look at the other packages you have that are tested this way, when enabling the lcov reporter to have an html output,

I'll accept anything that fixes this.

[onigoetz]

Sounds good.

Awesome, I'll finalize and test what I have and open the PR :)

Sigh, I thought rollup would dedupe that during the build.

Scratch what I said, I had a look on the other packages and indeed, it seems that Rollup takes the esm version of the dependencies which are all properly externalized in the other packages so no duplication happens.