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

Sourcemaps #3271

Merged
merged 11 commits into from
May 13, 2021
Merged

Sourcemaps #3271

merged 11 commits into from
May 13, 2021

Conversation

lukejacksonn
Copy link
Contributor

@lukejacksonn lukejacksonn commented May 4, 2021

Changes

This is a preliminary attempt at trying to get sourcemaps working in some capacity for single file (TS/JSX/TSX) esbuild and svelte plugin transforms. So far the most trivial way I have found of enabling these is through inline source maps.

I'm sure there are some downsides to inline source maps (especially in production) but during development they seem convenient as all the information that the browser requires to present mappings in the sources tab for any given file is contained within the file itself.

The changes here are pretty small if you exclude the testing directories which are just there for experimentation purposes and meant to be deleted before merging.

Here is a screenshot of the sources tab of a TSX project using react:

image

Here is a screenshot of the same project but with bundling enabled:

image

Here is a screenshot of the sources tab of a Svelte project:

image

Here is a screenshot of the same project but with bundling enabled:

image

Here is a screenshot of a vue project with sourcemaps:

image

General Observations

Some things I have noticed whilst testing these setups is that:

  • Individual files have inline sourcemaps during dev but when a project is bundled then esbuild seems to aggregate all source maps for all files in a bundle into an external source map that could be used in production (doesn't add weight to the bundle).
  • The above only appears to hold true for the basic esbuild case; when Svelte plugins are bundled an external sourcemap is generated but only for the index file (no aggregation of maps).
  • Errors seem to be thrown by the dev server when trying to resolve sourcemaps for Svelte HMR files.
[24:60:40] [snowpack] [404] Not Found (/_snowpack/pkg/internal.js.map)
[24:60:40] [snowpack] [404] Not Found (/_snowpack/pkg/hot-api-esm.js.map)
[24:60:40] [snowpack] [404] Not Found (/_snowpack/pkg/proxy-adapter-dom.js.map)
  • When generating a map for vue files, only a map for the template is created. Also only handles vanilla vue templates; there is a transform step in the snowpack vue plugin for jsx/tsx which is not covered by this change.

Testing

No automated testing yet.

Docs

No breaking changes, currently the inline sourcemaps will only be appended to files if buildOptions.sourcemaps === true.

@vercel
Copy link

vercel bot commented May 4, 2021

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/pikapkg/snowpack/5vtwJV86W965AcDPmquNoHf7PQQg
✅ Preview: https://snowpack-git-fork-lukejacksonn-sourcemaps-pikapkg.vercel.app


var charset = 'utf-8';
const mapping = (m) =>
'\n//# sourceMappingURL=data:application/json;charset=' +
Copy link

@benatshippabo benatshippabo May 6, 2021

Choose a reason for hiding this comment

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

question: it feels weird to manually concatenate the sourcemap url into the file, shouldn't whatever is being used to manipulate the files and generate the sourcemaps like magic-string, source-map, or recast be the one adding this in automatically?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree it does but the svelte compiler doesn't seem to expose an interface like esbuild does for defining wether you'd like the sourcemaps to be external or inlined; it just generates and outputs { code, map } presumably for use further on in the build process.

So I gathered we either need to write the map to disk/cache, create a URL for it and append that to the bottom of the file, or just append the source map itself. The latter seemed much easier at the time 😅

Copy link

@benatshippabo benatshippabo May 6, 2021

Choose a reason for hiding this comment

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

I am not too familiar with Svelte, but it seems strange that svelte's compiler doesn't add the sourcemap url at the end of the transform for users. Svelte's creator also made rollup and magic-string, so I would have thought that the Svelte framework would be sourcemap compliant.

Are we positive it isn't being hidden behind some Svelte compiler api flag? Have we tried using outputFilename compiler option and seeing if it appends the sourcemap url for us? https://svelte.dev/docs#svelte_compile

Copy link

@benatshippabo benatshippabo May 6, 2021

Choose a reason for hiding this comment

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

Looks like the esbuild svelte plugin also appends the url 😞 so I guess this is fine https://esbuild.github.io/plugins/#svelte-plugin

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for looking into this 🙇 very reassuring to know we are not alone in doing this appendage (I admit it did feel like a bit of a smell). Perhaps we make this a util function, something like inlineSourceMap(code, map).

@@ -42,7 +42,7 @@ export function esbuildPlugin(config: SnowpackConfig, {input}: {input: string[]}
jsxFactory,
jsxFragment,
sourcefile: filePath,
sourcemap: config.buildOptions.sourcemap,
sourcemap: config.buildOptions.sourcemap && 'inline',

Choose a reason for hiding this comment

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

suggestion: it might be cleaner if we just change the accepted values of config.buildOptions.sourcemap to be what esbuild accepts 'external' | 'inline' | 'both'

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree in principle although I couldn't get the browser to pick up any source maps when using any option other than inline hence I've limited it to just that at the moment.

Choose a reason for hiding this comment

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

Interesting, in that case we could just limit config.buildOptions.sourcemap to 'inline' | undefined for now

@benatshippabo
Copy link

I'm sure there are some downsides to inline source maps (especially in production)

question: should we also set sourcesContent: false if we detect that we are in production? https://esbuild.github.io/api/#sources-content

@lukejacksonn
Copy link
Contributor Author

That does sound like a good idea!

@lukejacksonn lukejacksonn marked this pull request as ready for review May 10, 2021 22:41
@lukejacksonn lukejacksonn requested a review from a team as a code owner May 10, 2021 22:41
Copy link
Contributor

@matthewp matthewp left a comment

Choose a reason for hiding this comment

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

lgtm

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants