allow --in-source-map inline #1490

alexlamsl · 2017-02-15T11:05:57Z

fixes #520

@kzc separated into two commits so it's easier to see the cli changes (haven't done anything to the API case yet)

You'll be glad to know that #520 (comment) didn't go to waste - aside from educating me on what a source map looks like, it forms the test case inside test/mocha/cli.js 😉

alexlamsl · 2017-02-15T11:12:50Z

... and just as I started to think about the API case, I just realised any changes to tools/node.js would massively conflicts with #1366 😓

CMTegner · 2017-02-15T12:19:02Z

bin/uglifyjs

+function read_source_map(code) {
+    var match = /\n\/\/# sourceMappingURL=data:application\/json(;.*?)?;base64,(.*)/.exec(code);
+    if (!match) {
+        print_error("ERROR: can't read source map");


Perhaps add "inline" to the message so it's easier to catch configuration errors (I'm thinking along the lines of confusing --source-map-inline with --in-source-map inline).

kzc · 2017-02-15T14:00:43Z

bin/uglifyjs

+    var match = /\n\/\/# sourceMappingURL=data:application\/json(;.*?)?;base64,(.*)/.exec(code);
+    if (!match) {
+        print_error("ERROR: can't read source map");
+        process.exit(1);


I don't think it should be a fatal error if the upstream code lacks an inline source map.

Perhaps something like "WARNING: inline source map not found" and return null; instead of process.exit(1);?

Done. Commit title updated.

kzc · 2017-02-15T14:46:48Z

@alexlamsl Thanks - nice job on the CLI. Surprisingly little new code added - mostly rearranging existing code. Glad the example proved useful.

I appreciate that adding this feature to minify() will massively conflict with #1366. Should we keep this PR in a [WIP] holding pattern until #1366 is merged?

The use of Buffer(..., 'base64') in this PR will create an incompatibility with #1366 anyway as it is not available in the browser without including a third party module (which we do not want to do). But I don't think the source-map module is compatible with the browser version of minify() anyway - @avdg can you confirm that?

Edit: If that's the case, then the exported base64 decode function for the browser can just throw a "not implemented" error as it won't be used.

kzc · 2017-02-15T14:58:42Z

@alexlamsl Would you mind changing the PR title and eventual commit description to "[WIP] allow --in-source-map inline"?

alexlamsl · 2017-02-15T15:33:16Z

I appreciate that adding this feature to minify() will massively conflict with #1366. Should we keep this PR in a [WIP] holding pattern until #1366 is merged?

Agreed.

The use of Buffer('...', 'base64') in this PR...

We actually have another problem in that we cannot use Buffer.from() due to Node.js 0.10.x compatibility.

I am assuming you are talking about tools/node.js with the browser use-case, as there's an existing usage of Buffer in bin/uglifyjs?

alexlamsl · 2017-02-15T15:37:20Z

I can't be certain, but at least in the browserified version of html-minifier there seems to be an implementation of Buffer embedded in the file. So I think we might be able to get away with this?

kzc · 2017-02-15T16:37:44Z

I am assuming you are talking about tools/node.js with the browser use-case, as there's an existing usage of Buffer in bin/uglifyjs?

No, I'm referring to the standalone uglifyjs --self code produced by #1366. Use of Buffer in tools/node.js is fine, but not in the portable code generated by --self. Notice the function exports that are substituted for the browser version in that PR:

https://github.com/mishoo/UglifyJS2/pull/1366/files#diff-465a5a39285d7d0ab958a973c3d58432R6

kzc · 2017-02-15T16:43:05Z

but at least in the browserified version of html-minifier there seems to be an implementation of Buffer embedded in the file. So I think we might be able to get away with this?

We want to avoid additional third party modules in Uglify where possible in order to keep Uglify small and fast. I'm fairly sure that source-map does not work in the portable --self version in the browser anyway, so it's not a loss of functionality.

kzc · 2017-02-15T16:51:45Z

We actually have another problem in that we cannot use Buffer.from() due to Node.js 0.10.x compatibility.

Are there any features of minify() or bin/uglifyjs that will not work in node 0.10.x?

base 64 decode appears to work correctly in node 0.10.x:

$ node -v
v0.10.41

$ node -p "Buffer('dGhlIHF1aWNrIGZveA==', 'base64').toString()"
the quick fox

alexlamsl · 2017-02-15T16:51:56Z

@kzc ah, I didn't notice #1366 exports the new minify.js - but yes, it copied the use of Buffer from tools/node.js, so it will fail if used inside a web browser.

And I've got confused with uglify-to-browserify which I ran into in the past. Sorry about that.

alexlamsl · 2017-02-15T16:53:16Z

Are there any features of minify() or bin/uglifyjs that will not work in node 0.10.x?

I was referring to the use of Buffer(...) which is now deprecated.

kzc · 2017-02-15T16:55:56Z

but yes, it copied the use of Buffer from tools/node.js, so it will fail if used inside a web browser.

I was under the impression that @avdg had tested that very thing. If that's the case, then #1366 would need some retooling because that's the point of its existence.

alexlamsl · 2017-02-15T16:58:05Z

I'd be positively impressed if sourceMapInline manages to just work inside the web browser.

kzc · 2017-02-15T16:58:59Z

I was referring to the use of Buffer(...) which is now deprecated.

I'm not too worried about that. It's still supported. No uglify user has complained as far as I know.

alexlamsl · 2017-02-15T17:06:29Z

So according to MDN we can use atob() and btoa() if Buffer is absent to provide the base64 codec, and at least IE10 supports that.

kzc · 2017-02-15T17:24:46Z

atob() does indeed work in Safari 9, and the latest versions of FireFox and Chrome:

alert(atob('dGhlIHF1aWNrIGZveA=='));

But it's not supported by node. So #1366 can check for its existence and patch exports.base64_decode (or whatever it may be called) accordingly.

kzc · 2017-02-15T17:32:56Z

@alexlamsl Your fix to optionally support inline input source maps works nicely:

$ echo 'foo(1+2);' | bin/uglifyjs - -cm toplevel --in-source-map inline --source-map-inline
WARN: inline source map not found
foo(3);
//# sourceMappingURL=data:application/json;charset=utf-8;base64,eyJ2ZXJzaW9uIjozLCJzb3VyY2VzIjpbIi0iXSwibmFtZXMiOlsiZm9vIl0sIm1hcHBpbmdzIjoiQUFBQUEsSUFBSSJ9

alexlamsl · 2017-02-15T17:37:27Z

Your fix to optionally support inline source maps works nicely

Thanks - I even have a test case for it 😉

kzc · 2017-02-24T00:10:45Z

Isn't this PR good to go?

alexlamsl · 2017-02-24T00:14:22Z

Not really if you mean as it is right now - tools/node.js has yet to be updated for this PR.

But delaying of #1366 meant I can now proceed in completing this. And it does looks like a good feature to have for 2.8.0 so I'll get to this right after I finished my local git housekeeping 😉

alexlamsl · 2017-02-24T01:44:50Z

@kzc due to new default optimisation options, I need to update test/input/issue-520/output.js to reflect the new output. PTAL to verify updated source map.

kzc · 2017-02-24T03:04:36Z

I never actually loaded these source maps to verify them, but it looks well formed as far as I can tell.

alexlamsl · 2017-02-24T04:24:51Z

One thing I'm confused with is --spidermonkey, where in cli it reads the file content the same way before parsing so we are covered, but with minify() that isn't the case.

kzc · 2017-02-24T04:36:04Z

It eventually does the same thing...

https://github.com/mishoo/UglifyJS2/blob/8898b8a0fe87f71c0ea2d35face6dfbf11db27ec/bin/uglifyjs#L388-L390

It's a bit confusing how TOPLEVEL can be either an Uglify AST or a Mozilla AST at different points in the program.

kzc · 2017-02-24T04:38:50Z

bin/uglifyjs also has to handle asynchronous input from stdin, while minify() does not.

kzc · 2017-02-24T04:51:20Z

Wait... I see what's going on: the first argument of minify() which happens to be named files is overloaded and assumed to be a Mozilla AST if options.spidermonkey is set. Confusing! We should add a test for it.

kzc · 2017-02-24T04:58:10Z

I knew that bin/uglifyjs could accept mozilla JSON from stdin, but I did not know it could accept mozilla JSON input from multiple files:

$ echo 'foo();' | uglifyjs --dump-spidermonkey-ast > foo.json
$ echo 'bar();' | uglifyjs --dump-spidermonkey-ast > bar.json
$ uglifyjs bar.json foo.json --spidermonkey
bar();foo();

Cool!

alexlamsl · 2017-02-24T08:11:42Z

@kzc ah, that means this PR won't work even for bin/uglifyjs when spidermonkey is involved.

Tempted to explicitedly throw an error for both interfaces if both spidermonkey and inSourceMap="inline" are specified for now. This way we minimise ambiguity, and if somebody ends up wanting to use that combination it can be extended in the future.

kzc · 2017-02-24T15:18:41Z

that means this PR won't work even for bin/uglifyjs when spidermonkey is involved.

I think Mozilla AST (spidermonkey) and inline input source maps are inherently incompatible because I don't think comments are preserved in the Mozilla AST - and if they are Uglify does not handle them.

https://github.com/estree/estree
estree/estree#94
estree/estree#41

alexlamsl · 2017-02-24T17:49:05Z

@kzc added checks & tests for built-in parser during inSourceMap="inline"

kzc · 2017-02-24T19:30:15Z

Great. I'm sure it's fine.

- limited to one input file (or `stdin`) - only works with built-in parser fixes mishoo#520

Forkest · 2017-02-25T08:12:10Z

Don't you think that having
--source-map-inline and
--in-source-map inline results in a bit incosistent API?
Maybe change the new option to a version with a dash before it is too late?

kzc · 2017-02-25T08:43:38Z

I could live with --in-source-map-inline, but if --in-source-map=foo.js.map is also specified which takes precedence?

Forkest · 2017-02-25T08:50:44Z

The behavior should be the same as with --source-map=foo.js.map --source-map-inline. Tried that, inline option takes precedense, file option is ignored.

kzc · 2017-02-25T15:48:09Z

inline option takes precedence

Seems the choice was arbitrary. Probably should error out for a conflict between --source-map and --source-map-inline.

At least with the present behavior of --in-source-map [filename|inline] there is no potential conflict. But whichever way, it should be consistent. No reason why we couldn't also support --source-map=inline to output an inline source map in addition to adding --in-source-map-inline.

Perhaps even have extra aliases --out-source-map and --out-source-map-inline respectively to further reduce confusion as to the direction of the source map operation which has tripped me up in testing on more than one occasion.

alexlamsl changed the title ~~allow inlined source maps in compiled JS~~ [WIP] allow inlined source maps in compiled JS Feb 15, 2017

CMTegner reviewed Feb 15, 2017

View reviewed changes

kzc reviewed Feb 15, 2017

View reviewed changes

alexlamsl changed the title ~~[WIP] allow inlined source maps in compiled JS~~ [WIP] allow --in-source-map inline Feb 15, 2017

alexlamsl force-pushed the issue-520 branch from ea83220 to f930a9f Compare February 15, 2017 15:52

kzc mentioned this pull request Feb 15, 2017

[WIP][3.x] Make minify portable by making it work without file system #1366

Closed

9 tasks

alexlamsl force-pushed the issue-520 branch from f930a9f to 614ea47 Compare February 24, 2017 01:42

This was referenced Feb 24, 2017

Is it possible to preserve inline sourcemaps? #780

Closed

Can't preserve sourceMap comment line #754

Closed

allow --in-source-map inline

4199be9

- limited to one input file (or `stdin`) - only works with built-in parser fixes mishoo#520

alexlamsl force-pushed the issue-520 branch from 4af064d to 4199be9 Compare February 24, 2017 20:02

alexlamsl changed the title ~~[WIP] allow --in-source-map inline~~ allow --in-source-map inline Feb 24, 2017

alexlamsl merged commit cf0951f into mishoo:master Feb 24, 2017

alexlamsl deleted the issue-520 branch February 24, 2017 20:11

alexlamsl mentioned this pull request Mar 23, 2017

Error parsing arguments for flag `b' 2.8.15 #1637

Closed

kzc mentioned this pull request Mar 28, 2017

Is it possible to add support for inline source maps when code is piped? #1722

Closed

allow --in-source-map inline #1490

allow --in-source-map inline #1490

Conversation

alexlamsl commented Feb 15, 2017

alexlamsl commented Feb 15, 2017

CMTegner Feb 15, 2017

Choose a reason for hiding this comment

alexlamsl Feb 15, 2017

Choose a reason for hiding this comment

kzc Feb 15, 2017

Choose a reason for hiding this comment

alexlamsl Feb 15, 2017

Choose a reason for hiding this comment

kzc commented Feb 15, 2017 • edited Loading

kzc commented Feb 15, 2017

alexlamsl commented Feb 15, 2017

alexlamsl commented Feb 15, 2017

kzc commented Feb 15, 2017

kzc commented Feb 15, 2017

kzc commented Feb 15, 2017

alexlamsl commented Feb 15, 2017

alexlamsl commented Feb 15, 2017

kzc commented Feb 15, 2017

alexlamsl commented Feb 15, 2017

kzc commented Feb 15, 2017

alexlamsl commented Feb 15, 2017

kzc commented Feb 15, 2017

kzc commented Feb 15, 2017 • edited Loading

alexlamsl commented Feb 15, 2017

kzc commented Feb 24, 2017

alexlamsl commented Feb 24, 2017

alexlamsl commented Feb 24, 2017

kzc commented Feb 24, 2017

alexlamsl commented Feb 24, 2017

kzc commented Feb 24, 2017

kzc commented Feb 24, 2017

kzc commented Feb 24, 2017 • edited Loading

kzc commented Feb 24, 2017 • edited Loading

alexlamsl commented Feb 24, 2017

kzc commented Feb 24, 2017

alexlamsl commented Feb 24, 2017

kzc commented Feb 24, 2017

Forkest commented Feb 25, 2017

kzc commented Feb 25, 2017

Forkest commented Feb 25, 2017 • edited Loading

kzc commented Feb 25, 2017

kzc commented Feb 15, 2017 •

edited

Loading

kzc commented Feb 15, 2017 •

edited

Loading

kzc commented Feb 24, 2017 •

edited

Loading

kzc commented Feb 24, 2017 •

edited

Loading

Forkest commented Feb 25, 2017 •

edited

Loading