Fix source map generation

[krisselden]

Ensure mappings are emitted for AST locations so that mapping back to the original source map with a tool like sorcery is possible.

[krisselden]

Fixes #24

[krisselden]

@Rich-Harris even after this change sorcery still doesn't work, does magicString.addSourcemapLocation( need to be done before any mutation on MagicString?

[Rich-Harris]

hmm... it should Just Work™. Might well be an unrelated bug – I need to spend some time on that project, will add this as an issue. If you're able to help out with a reduced test case that would be very helpful, thanks!

[nolanlawson]

Except for the necessary README changes, everything looks great to me! Although based on your comment, are you saying this doesn't actually resolve the issue?

[nolanlawson]

So it looks like this includes three new options - file, source, and includeContent. Could you update the README please? :)

[krisselden]

Will do, but I'm currently trying to get it to work with jquery, it currently did come close to working with ember.

The issue is this needs to be done after minify and a minified map is quite complicated. I'll push up a demo of the issue.

[krisselden]

@nolanlawson it did not, I'm guessing there are some edge cases with uglify's map that magic string or sorcery is not handling and @Rich-Harris answer to magic string not using an input map is to use sorcery, as far as I can tell this PR aligns with how rollup chains source maps.

[krisselden]

So having trouble reducing it simple examples work well as is, this PR for certain is an improvement over the existing.

I'll keep working on it, still not sure if it is socery, uglify or magic string. As I said this PR at least gets it to work in most cases. Maybe we should return {code, map} instead of inlining? Since it would be for sure something you'd want split out in prod

[krisselden]

@Rich-Harris @nolanlawson It is the fallback from extra mappings that don't trace back to the uglified map https://github.com/krisselden/sourcemap-issue

[krisselden]

@nolanlawson so I think the simplest would be just to have an option that returned the magic string instance.

Unfortunately I think this is an issue with sorcery and rollup, but both do something different here to trace back to the original source that requires different addSourcemapLocation calls. The fallback to line start in sorcery for missing mappings causes issues if you have extra mappings where rollup just drops the mapping but either case I wish the assumption was to merge the mappings so you didn't need any extra mappings, but only mappings for edits but I think this is an issue with rollup and sorcery not this tool.

To get the right result in sorcery I have to walk only the mappings in the uglified map and call addSourcemapLocation location only for those mappings, which means I need the magic string before it is converted to a map.

[nolanlawson]

From this conversation it sounds like this is a bug in magicstring. I'm pretty hesitant to complicate the API by returning the magicstring instance; it feels so me like emitting the sourcemap ought to be good enough. 😕

[krisselden]

I understand the concern, the issue is this has to be done post minify and I don't know of any tool that can remap the sourcemap back to the input correctly.

The API for magicstring that lets you mark locations at least lets you make it the same resolution as the input map so it is easier to remap to an input.

Maybe an acceptable API would be to take an input map? Also, it would be nice if the return value was {code: string, map: Object}

.

[ansballard]

Floating over from #5. Reading through the comment chain here, it looks like the suggested change is to pass in and emit source maps. Is anyone following this currently working on a pull request? I don't have time to dive into this for a bit, but I do have a mostly working rollup plugin that's just waiting on sourcemap support.

There's also a comment about it possibly being a magicstring issue. If that's the case, has anyone filed an issue on the magicstring repo?

Just seeing if I can help get some momentum going, thanks!

[krisselden]

@ansballard there is no issue with magicstring itself, it correctly provides a way of marking where the input mappings are with addSourcemapLocation which allows you to make the resolution correct.

Without an input map, the correct thing to do as this PR does is add the locations for the nodes.

The only issue is with this approach is if you try to use sorcery to map back to the original input, if you added a mapping that doesn't exist, it does not drop the mapping like rollup does, it maps it to the beginning of the line which produces a unusable sourcemap.

The best thing would be to at least be able to pass in the locations of the mappings you want to remap or return the magic string itself so you can do this yourself.

[krisselden]

@ansballard also, just to clarify, no tool I know of would map what this is currently outputting back to the minified sourcemap and this tool needs to be run after minification.

Contrary to what most people think, you need to provide mappings for all the expressions and names for the identifiers in a sourcemap, not just your edits, to have the sourcemap function well.

[nolanlawson]

Hi all, I don't really have the time or motivation to maintain optimize-js anymore, and also it is quite likely that it will become obsolete, as I know both V8 and Chakra are in the midst of improving their function-parsing heuristics.

So I've gone ahead and made @krisselden a GH contributor with npm owner access; please do as you wish. :) Sorry for being so slow with these PRs!