SourceMap issues

[nwidger]

I've been trying to debug JavaScript code running in goja that was compiled from TypeScript using the official TypeScript compiler and have run into some issues with goja's support for source maps. I think I've found four problems:

• parser.ParseFile does not look for the source map relative to the parsed file's containing directory when the source map URL is relative. Here's a Playground link which reproduces the issue. According to the source map spec, When the source mapping URL is not absolute, then it is relative to the generated code’s “source origin”.

• parser.ParseFile expects to find the //# sourceMappingURL= line in a very specific location in the file, which breaks when the file is loaded as a module by github.com/dop251/goja_nodejs/require since the file's source gets wrapped in (function(exports, require, module) { }). Here's a Playground link reproducing the issue. One possible solution would be to be less strict when looking for the //# sourceMappingURL= line. At least in Node.js, they seem to just look for any occurrence of the line in the source file. Another possible solution could be to parse the non-wrapped source with parser.ParseFile in require.Registry.getCompiledSource, stash the returned *ast.Program's SourceMap field, then parse the wrapped source with parser.ParseFile and overwrite it's SourceMap before passing it to goja.CompileAST. But I do not know whether this is a valid, or even safe, solution.

• github.com/dop251/goja_nodejs/require exposes a SourceLoader type to allow reading files using a mechanism other than ioutil.ReadFile as used by DefaultSourceLoader, but this mechanism is completely unknown to parser.ParseFile when it tries to find the source map, and thus always passes the source map's path to os.Open.

• goja.SrcFile ignores the source return value when calling f.sourceMap.Source() and therefore assumes that the filename of the source and generated file are the same. When compiling from TypeScript to JavaScript, I believe this means that even if goja finds and loads the source map, stack traces print the JavaScript filename but using the row and column numbers corresponding to the TypeScript file. I'm not sure if it should be optional to have stack traces refer to the original source file rather than the generated file when a source map is found, but I would think that the stack trace should at least be consistent.

I'm happy to open separate issues for any of these problems, or to open them in the goja_nodejs repo if you think they belong there instead. But first I was hoping to get your thoughts on whether these issues are valid. I'd be happy to try to open a PR for any of them if you agree they should be fixed.

[dop251]

Can you try https://github.com/dop251/goja/tree/source_map_enhancements and https://github.com/dop251/goja_nodejs/tree/source_map_enhancements and let me know if it helps?

[nwidger]

This is great, your two branches fix all the issues for me except the last bullet point. Thank you so much, these changes are going to make debugging so much easier. I wish I could buy you a beer or something. :)

As for the last issue, I'm happy to take a stab at it if you aren't interested in tackling it yourself. I was thinking that, since the generated JavaScript file goja is running may have been created from multiple source files, a Name field would need to be added to goja.Position. This would be filled in by (*goja.SrcFile).Position() using the source return value from (*sourcemap.Consumer).Source(), joined with the filepath.Dir() of (*goja.SrcFile).name. Finally, (*goja.StackFrame).Write() would print Position.name instead of f.prg.src.name. Anyway, that would probably be my first approach.

[dop251]

I missed the last point as I was looking at the original notification email and it was apparently added later, sorry about that.
Nevertheless, I spotted the problem myself and was going to have a look at it later. There is a bit of an overlap between file.File and goja.SrcFile, I was going to get rid of the latter and move the functionality to the former, and also encapsulate the SourceMap handling in there.

[nwidger]

No need to apologize, I made some ninja edits to the issue and probably should have made that more apparent, my bad. Glad you noticed the last issue, too, let me know if I can help in any way. I'm happy to test out any solution you come up with.

[dop251]

I have updated the branch, could you try now?

[nwidger]

I just gave the branch a whirl and it seems to work like a charm. Unless there are specific cases you'd like me to test out I think all the problems I ran into are now fixed. Thanks so much for doing all this work!

[dop251]

No problem. The changes have been merged so I'm closing this now.