[CS2] Fix v3 source map

[GeoffreyBooth]

Per discussion in #3075, in the v3SourceMap object:

• Always now return null instead of empty strings, or an empty array instead of [''].
• If options.sourceFiles (an undocumented Node API option) is unspecified, but options.filename is, generate the v3SourceMap sources field from [options.filename]. The API by definition is always only compiling one file.

I don’t know if it’s worth documenting the source map generatedFile (maps to v3SourceMap file) or sourceFiles (maps to sources) or sourceRoot (maps to same) fields? They seem pretty obscure.

[lydell]

This all sounds very sensible, but I can't know for sure what's correct. IMO, feel free to merge if you feel confident.

[GeoffreyBooth]

I don't feel confident. I thought you were a contributor to source-map? 😄

[lydell]

Yep, but there's been a while since I did anything in source-map. And what I learned when contributing there is that it is really difficult to get the edge cases right, because those things aren't really specified so it's mostly about getting the source maps to work with other tools and browers...

The more I think about this though, I think we could merge it. Worst case scenario we just revert it if something comes up?

[lydell]

Hmm, but as far as I remember, every source mapping always points to an index in the sources array, so perhaps it does not make sense to output an empty sources array. Something like ["(unknown)"] might be better...

[GeoffreyBooth]

I think I'll revert the change to empty strings. I don't want to mess up downstream tools.

What about sources? It works fine for CLI use, it's only an empty string for Node API use; which in a way makes sense, as the compiler is given a string rather than a file. I think the filename is more useful, so that the browser has something to call the sources, but I could see it either way. What do you think?

[lydell]

The file and sourceRoot fields are optional, so I think it makes sense to set them to null if missing.

Regarding the sources array, I think we should default to ["whateverMightBeUsefulToTheUser"] rather than an empty array. The point of a source map is showing from where compiled code comes, so having an empty sources array does not make sense. Then it is better to have no source map at all. I'm not sure what you mean by "I think the filename is more useful, so that the browser has something to call the sources, but I could see it either way".

[GeoffreyBooth]

I meant that we're only discussing the behavior when someone calls .compile via the API; when someone compiles via the CLI, sources is fine. When someone calls .compile, the input is a string rather than a filename, so in that sense it follows that there would be no “source” (in that sources are always file paths or URLs). But just because that makes logical sense doesn’t mean it’s useful. I think it would be better to throw the supplied filename into sources if the user gave us one.

If they didn’t provide one, though, then is it better for sources to be [""] (current behavior) or [] (new behavior)? If you like the new behavior, then I guess this PR is okay to merge in as it is.

[GeoffreyBooth]

Here’s an example of one downstream tool interacting with these fields: https://github.com/meteor/meteor/blob/devel/packages/non-core/coffeescript-compiler/coffeescript-compiler.js#L29

This PR wouldn’t break that one, at least. I don’t know what downstream tools would break, unless any are checking for empty strings.

[lydell]

I think it would be better to throw the supplied filename into sources if the user gave us one.

I don't remember the API exactly. What is this supplied filename?

then is it better for sources to be [""] or []

[""] is better, ["(unknown)"] is better yet, using filename (if appropriate) is best. Many compilers let the user pass in not only the string to compile, but also the source file name for this reason.

[GeoffreyBooth]

If we’re going to return [""] for sources if we’re missing a filename, does it then make sense to return null for file and sourceRoot if those are missing? Maybe we should just keep the current behavior?

See http://coffeescript.org/v2/#nodejs-usage. On the options object we encourage a filename key to be set, that we say gets passed along as part of the source map.

[lydell]

options.filename, string: the filename to use for the source map.

Ah, there it is. Isn't that already used for the sources?

[lydell]

The file and sourceRoot fields are optional, so I think it makes sense to set them to null if missing.

[lydell]

Sigh. Source maps are always so confusing.

[GeoffreyBooth]

I’m leaning closer and closer to “no one is complaining that this is broken.”

[lydell]

You mean that you'd rather close this and #3075? I guess that's fine, too.

[GeoffreyBooth]

I just don’t know. I feel like if we’re going to change/fix this, we should just do it, especially now before 2.0.0. But on the other hand there aren’t any open issues about this, so if we change this and any downstream tools were relying on this (broken) behavior, we could break them. Then again, breaking them as part of a 2.0.0 release would be the time to do so.

I think likely any downstream tools were just working around these missing fields (as Coffeeify did) or using the undocumented generatedFile/sourceFiles/sourceRoot options (as Meteor did). Since you’re already calling from API, it’s easy to mutate the v3SourceMap object that gets returned, before passing it along to the browser or saving it or whatever. So the fact that this was broken probably didn’t bother many people.

You’ve worked with source maps much more than I have. What do you think we should do?

[GeoffreyBooth]

How about we do what TypeScript does? (Based on this.)

ts = require 'typescript'

source = "let x: string  = 'string'"

result = ts.transpileModule source,
  compilerOptions:
    module: ts.ModuleKind.CommonJS
    sourceMap: yes

console.log JSON.parse result.sourceMapText

{ version: 3,
  file: 'module.js',
  sourceRoot: '',
  sources: [ 'module.ts' ],
  names: [],
  mappings: 'AAAA,IAAI,CAAC,GAAY,QAAQ,CAAA' }

So:

• For file, it just makes up a filename. Not sure if this is a practice we should follow.
• For sourceRoot, it uses an empty string.
• For sources, it produces a one-element array with the made-up filename.
• For names, it uses an empty array.

I’m fine with sourceRoot and names. For file and sources, I feel like we might as well use the filename we’re given if the user provides one via the filename field. But if they don’t . . . I feel like we should either use an empty string, like the current behavior, or <anonymous> like the browser compiler uses. Thoughts?

[lydell]

I like the idea of copying the approach of another big project.

For file, it just makes up a filename. Not sure if this is a practice we should follow.

I'd rather default to an empty string. (Note this is supposed to be the name of the generated JS file. I'm not sure if any tools actually even use this field.)

For sourceRoot, it uses an empty string.

👍

For sources, it produces a one-element array with the made-up filename.

I think it's fine to default to [options.filename], and if missing ['<anonymous>'].

For names, it uses an empty array.

👍