`sourceMapUrl` fallback in `ImporterResult` creates bloated source map output with source contents as source urls

[ntkme]

• Proposal (Enhance sourceMapIncludeSources option sass#4170)
• Deprecation
  • Specification
  • Tests (Enhance sourceMapIncludeSources option sass-spec#2097)
  • Dart Sass (Enhance sourceMapIncludeSources option #2713)
  • Embedded host (Enhance sourceMapIncludeSources option embedded-host-node#411)
• Breaking change
  • Specification
  • Tests
  • Dart Sass
  • Embedded Host

---

dart-sass/lib/src/async_import_cache.dart

Lines 371 to 375 in 62ec662

	/// Returns the URL to use in the source map to refer to [canonicalUrl].
	///
	/// Returns [canonicalUrl] as-is if it hasn't been loaded by this cache.
	Uri sourceMapUrl(Uri canonicalUrl) =>
	_resultsCache[canonicalUrl]?.sourceMapUrl ?? canonicalUrl;

In ImportCache, the sourceMapUrl always fallback to canonicalUrl for null.

dart-sass/lib/src/importer/result.dart

Lines 20 to 28 in 62ec662

	/// An absolute, browser-accessible URL indicating the resolved location of
	/// the imported stylesheet.
	///
	/// This should be a `file:` URL if one is available, but an `http:` URL is
	/// acceptable as well. If no URL is supplied, a `data:` URL is generated
	/// automatically from [contents].
	Uri get sourceMapUrl =>
	_sourceMapUrl ?? Uri.dataFromString(contents, encoding: utf8);
	final Uri? _sourceMapUrl;

However, in ImporterResult it already fallbacks to a data url for null, so in the ImportCache it will never actually fallback to the canonicalUrl.

This causes undesired bloats with sourceMapIncludeSources: true, where the source contents are included and the sourceMapUrl are encoded version of the exact same source contents. On the other hand, with sourceMapIncludeSources: false, the source map can still contain the source contents via data uri, despite being explicitly told to not expose contents.

The comments around the original code makes be believe the intention was to provide source map for source map url that's not file: protocol. However, modern browser can handle non-file: url in source map just fine, especially when source contents are present.

[nex3]

I do think there's a logic mismatch in here somewhere, but I'm not sure this is sufficient to solve it. As much as possible, we want the default options (or the defaults when only some options are specified) to do something useful. For source maps, that means we want to make sure as much as possible that by default when the user tries to generate source maps, they'll get a source map that allows a consumer to identify the original source code in practice.

The problem with falling back to canonical URLs is that, in general, canonical URLs are useless for locating source files. Consumers have no way of interpreting them. The notable exception is file: URLs, but those already set an explicit source map URL and thus aren't relevant to this issue.

Now, canonical URLs can be useful for identifying source files. If the caller opts to include the source file contents, URLs no longer have a purpose in terms of actually locating resources; as far as tools are concerned, they're opaque identifiers in a one-to-one mapping with source files that can be surfaced to users to help them understand the relationship between those files. But, crucially, that's not the default mode Sass uses when generating source maps. If the user just specifies sourceMaps: true in the JS API or Dart APIs, the default behavior is to load all source files by URL, which means that it makes sense for the code to be written with the expectation that URLs are intended to provide access to the source code, not merely to identify it.

I think the underlying issue here is that it just doesn't make sense, semantically, to request that the compiler generate source maps without embedding sources without also telling it which URLs to use for those sources (via ImporterResult.sourceMapUrl). But that's also currently the default behavior, because we treat "embed sources" as a flag meaning specifically "include all sources in sourcesContent" without any reference to the broader context.

I think the correct solution here would be to give the "embed sources" option three possible values:

• never, meaning that the source text of stylesheets is never included in source maps, either through sourcesContents or via a data:` URL.
• auto, meaning that the source text of a stylesheet is included in sourcesContent only if its importer doesn't provide an explicit sourceMapUrl.
• always, meaning that the source text of a stylesheet is always included in sourcesContent regardless of whether its importer provides an explicit sourceMapUrl.

If we had this distinction, we would no longer be conflating an absent "embed sources" option with one that's explicitly requesting no embedding. We could safely fall back on using the canonical URL as the source map URL, because we'd still embed the source file contents regardless, and we could stop generating data: URLs because we have the ability to selectively include sourcesContents. It would require a change to the JS API spec, though.

[ntkme]

For source maps, that means we want to make sure as much as possible that by default when the user tries to generate source maps, they'll get a source map that allows a consumer to identify the original source code in practice.

I have actually thought about this, and an ugly middle ground without breaking the existing user experience can be: move the fallback logic into ImportCache, conditionally fallback to canonical urls for sourceMapIncludeSources: true, and data: urls for sourceMapIncludeSources: false. However, this is introducing more inconsistent conditional behavior that I don't really like, but well it avoids breaking the current user experience and avoids adding new options.

The user experience problem without sourceMapIncludeSources: true is real, but in my opinion that's a problem between a user serving the source files and DevTools resolving the source files, rather than an issue of sass trying to help in between with a layer of indirection. For example, setting a http url for source map would work only if the source map is served via the server with proper config. Would it be better to step in and just replace a potentially not loading http url with data url? The answer is probably no, as the better solution to that problem is sourceMapIncludeSources: true.

The following screenshot is an example of nanoc produced dart-sass source map using canonical urls. The non-resolvable canonical urls with sourceContents actually works very well in DevTools, in fact looks far better than data: url, despite fact that the absolute urls are misinterpreted as relative paths by DevTools.

[nex3]

I'm not too concerned about breaking changes; if we make sourceMapIncludeSources in JS take a bool | "always" | "auto" | "never' | undefined for now and deprecate bool we're fine on that front. We can probably even do something similar in the embedded API where 0 is interpreted as "auto", 1 as "always", and 2 as "never".

Be aware that source maps aren't just consumed by dev tools, and we can't rely on something being acceptable in dev tools to mean that it'll be acceptable anywhere. The other major use-case is post-processors, where we want diagnostic messages to be cleanly associated with the original Sass source code that generated them. But in general, we should expect that consumers will use source maps in ways we can't anticipate beyond the guarantees of the specification.

[ntkme]

We can probably even do something similar in the embedded API where 0 is interpreted as "auto", 1 as "always", and 2 as "never".

That's smart. Protobuf encode bool as varint, so we can just change the definition to varint and keep the protocol backward compatible.

So:

sourceMapIncludeSources	has explicit sourceMapUrl?	effective sourceMapUrl	sourceContent
auto (undefined, false)	false	data: url canonicalUrl	false true
auto (undefined, false)	true	sourceMapUrl	false
always (true)	false	data: url canonicalUrl	true
always (true)	true	sourceMapUrl	true
never (false)	false	canonicalUrl	false
never (false)	true	sourceMapUrl	false

[nex3]

I would say the first row should include sourcesContent and should use canonicalUrl as the effective source map URL. Otherwise that looks right to me.

[ntkme]

Updated the table. I spilt the auto and false into two different options, that false would retain current behavior as is. Question is that whether it's necessary to keep false behave exactly as it was, or should we just update false to behave like auto, which I think vast majority of, if not more, tools out there should be able to handle.

[ntkme]

Updated table again. I think it makes sense to just merge auto and false because currently false can make source content available anyways, and the data uri is less efficient than just embed the raw source. Treating them the same way also significantly reduces the code complexity of passing options around.

[nex3]

Yes, I agree that once we're being more thoughtful about using sourcesContent there shouldn't ever be a reason to emit data: URLs.

[ntkme]

I decided it's probably a good idea to take advantage that we implicitly support undefined, that I updated the table to map undefined to 'auto', true to 'always', and false to 'never' for JS API. I think this would provide better ergonomics - most users using default without setting the option will get auto which optimizes for out of box user experience as before, and users who explicitly set false will get never as intended.

For Dart API, this introduces a brand new option. Because previously it always generates a SingleMapping with all source contents, the default for the new option is always for backward compatibility. When auto or never is set, some or all source contents will be set to null in post processing before returning result.

For CLI, only always (true) / never (false) are supported in order to avoid a breaking change to command line options. Because CLI mode does not support user defined importers, auto will only make a difference for --stdin which is a pretty rare use case anyways.

PRs:

• Enhance sourceMapIncludeSources option sass#4170
• Enhance sourceMapIncludeSources option #2713
• Enhance sourceMapIncludeSources option embedded-host-node#411
• Enhance sourceMapIncludeSources option sass-spec#2097