sourceMap.content "auto" option #298

connorjclark · 2019-03-02T22:05:25Z

I originally made these changes on UglifyJS2, but I merged them to Terser in the hopes they'd get accepted here :)

My original PR on UglifyJS: mishoo/UglifyJS#3220

Also involved adding these changes: mishoo/UglifyJS#3058

overview

--source-map "content=auto"

Allows for easier composition of source maps.

If an input file specifies a sourceMappingURL comment, the source map will be fetched.
If there is no comment, given an input file of src/file.js, the map will be looked for
at src/file.js.map and src/file.map. Maps can fetched via HTTP too.

--source-map "content=auto,contents=file1.js*file1.js.map|file2.js*path/to/map.js.map|..."

Allows for overriding the auto resolution strategy.

other stuff

Open to a different format for sourceMap.contents.

This is a super set of the other source map content options. I suggest removing those in a new major version, and making this "auto" behavior the default.

…guration

L2jLiga

Cool stuff 😄

L2jLiga · 2019-03-04T02:35:06Z

bin/uglifyjs

@@ -221,6 +223,59 @@ function run() {
    } catch (ex) {
        fatal(ex);
    }
+    if (content == "auto") {
+        var to_ascii = function(b64) {


I think we can use UglifyJS.to_ascii

L2jLiga · 2019-03-04T02:36:07Z

bin/uglifyjs

@@ -221,6 +223,59 @@ function run() {
    } catch (ex) {
        fatal(ex);
    }
+    if (content == "auto") {


I would like to use strict equal

L2jLiga · 2019-03-04T02:39:47Z

bin/uglifyjs

+
+            if (sourceMappingURL.indexOf('http') == 0) {
+                try {
+                    return execSync('curl -s ' + sourceMappingURL).toString();


On my windows machine curl is not installed.... 😖

Replace with the http libs. Required some promises.

L2jLiga · 2019-03-04T02:48:24Z

lib/minify.js

+            if (typeof source_map_content == "string" && source_map_content != "inline" && source_map_content != "auto") {
+                source_map_content = parse_source_map(source_map_content);
+            }
+            source_maps = source_map_content && Object.create(null);


According to #300 (comment) I guess it would be much better to use smth like:

if (source_map_content) source_maps = new Set();

At least it will reduce variety in code which doing same things

👍 , but it's actually a Map.

L2jLiga · 2019-03-04T02:55:02Z

bin/uglifyjs

+
+            if (!sourceMappingURL) {
+                // infer
+                if (fs.existsSync(name + '.map')) {


ESLint: Strings must use doublequote. (quotes)

L2jLiga · 2019-03-04T02:56:36Z

lib/sourcemap.js

-            if (sourceContent) {
-                generator.setSourceContent(source, sourceContent);
-            }
+    var maps = options.orig && Object.create(null);


It looks like Set

👍 , but it's actually a Map.

L2jLiga · 2019-03-04T03:05:07Z

bin/uglifyjs

@@ -135,7 +136,7 @@ if (program.output == "ast") {
 if (program.parse) {
    if (!program.parse.acorn && !program.parse.spidermonkey) {
        options.parse = program.parse;
-    } else if (program.sourceMap && program.sourceMap.content == "inline") {
+    } else if (content == "inline") {


I'm little bit worried about this changes..
ESLint: 'content' is not defined. (no-undef)

connorjclark · 2019-03-04T05:47:16Z

bin/uglifyjs

@@ -4,7 +4,7 @@

 "use strict";

-require("../tools/exit.js");
+// require("../tools/exit.js");


I had to remove this to get the tests that expect errors to work. I can't reason about this code or how to augment it - ideas?

connorjclark · 2019-03-04T05:49:53Z

This is a super set of the other source map content options. I suggest removing those in a new major version, and making this "auto" behavior the default.

Should this go for 4.0 or 5.0?

connorjclark · 2019-03-04T05:52:56Z

Forgot node 6 doesn't do async/await :( I'll update tomorrow. although, don't you think terser users would be long off of 6? :)

L2jLiga · 2019-03-04T06:14:53Z

I think it can be part of Terser 4.0

connorjclark · 2019-03-04T06:21:55Z

I think it can be part of Terser 4.0

OK. I'll see about removing the "inline" in a follow up PR.

connorjclark · 2019-03-04T17:51:01Z

OK- removed async/await. Obviously made the code less readable. More proof that async/await is a game changer :)

connorjclark · 2019-03-04T18:19:40Z

bin/uglifyjs

+        resolveSourcemapsPromise = Promise.resolve();
+    }
+    return resolveSourcemapsPromise.then(() => {
+        var result = UglifyJS.minify(files, options);


all of this below is just indentation

connorjclark · 2019-03-06T19:19:28Z

What to do about #298 (comment) ?

L2jLiga · 2019-03-07T01:58:58Z

I don't know, I guess it's a hack for old libUV and it can be deleted, not sure..

@fabiosantoscode, @kzc could you clarify?

fabiosantoscode · 2019-03-10T17:45:03Z

@hoten @L2jLiga node isn't guaranteed to output to stdout/stderr properly if you don't do that workaround. The workaround for that is to change instances of process.exit(errCod) with process.exitCode = errCod and change the code flow as necessary.

fabiosantoscode · 2019-03-10T17:48:30Z

See the code example reading "this is an example of what not to do" https://nodejs.org/api/process.html#process_process_exit_code

connorjclark · 2019-03-10T21:08:55Z

Interesting, never ran into that before. I tweaked the exit workaround code so it'll also work within a promise context, without causing an unresolved rejection (the throw was problematic).

connorjclark · 2019-03-15T19:55:27Z

Closing / Opening to force travis rerun. They had issues when this last ran.

connorjclark · 2019-03-15T20:56:56Z

'istanbul' is not recognized as an internal or external command

OK, travis env is borked.

L2jLiga · 2019-03-15T23:55:47Z

@hoten, you can tweak "test" script to exclude Istanbul call and make build green

L2jLiga · 2019-03-17T06:28:34Z

@hoten there're conflicts after #300 and #288

connorjclark · 2019-03-18T17:03:55Z

Resolved conflicts.

connorjclark · 2019-03-21T00:02:05Z

Good to merge?

fabiosantoscode · 2019-04-14T19:25:20Z

I've just looked through the PR. It has some issues like generated code in test files, the fact that the exit() function is synchronous or asynchronous depending on the situation, regex-based reading of things you wouldn't normally use a regex for...

And I was adding comments one by one to outline them, but I realized the PR can't be accepted because of this:

#298 (comment)

If we're going to add this feature, it makes no sense to add it only in the CLI. The maintenance cost is too high and most people are using Terser behind another tool anyway. We can't add it to the CLI and the API at the same time, given the fact that the API is synchronous (IE Terser.minify returns an object, not a Promise). Making the Terser API asynchronous would have a huge impact on Terser users, making them use promises in scripts that might have always been synchronous. I can't push this upon them.

In conclusion, this is an amazing idea in theory, but without a real option to do a synchronous HTTP request present in node core, in practice it doesn't work.

connorjclark · 2019-04-15T16:10:31Z

Let's not close this just yet :) Your issues aren't unresolvable.

If we're going to add this feature, it makes no sense to add it only in the CLI.

Resolving source maps when not explicitly provided by poking around the environment (either the local filesystem or by reaching out to the web) only makes sense in the CLI. AFAICT, bin/uglifyjs is free to be as node-y as it wishes, while everything in lib (the API) must be shippable to a web context (and be synchronous).

Granted, the name auto makes less sense in isolation for the API (it's really looking up source maps in a provided map of them: options.sourceMap.contents) so perhaps auto would be better named provided (while still being automatically resolved from the CLI entry).

The maintenance cost is too high and most people are using Terser behind another tool anyway.

I disagree about the maintenance cost being high, but could you explain further?

The fact that Terser does not handle multiple source maps from multiple inputs is the exact reason this PR exists. Having this tool in my asset pipeline (well, it was uglifyjs, but still) made proper usage of source maps impossible. Good source map support is becoming very important, and Terser shouldn't ignore that.

We can't add it to the CLI and the API at the same time, given the fact that the API is synchronous (IE Terser.minify returns an object, not a Promise).

I'm not seeing the issue here. Terser.minify still returns an object in this PR. The API is still synchronous - the CLI entry point is not, but that is not the API, lib is.

Making the Terser API asynchronous would have a huge impact on Terser users, making them use promises in scripts that might have always been synchronous. I can't push this upon them.

I agree, lib should remain synchronous. But let's not stick to such standard for bin, an entry point no one should be relying on as a library, except via node bin/uglifyjs ....

In conclusion, this is an amazing idea in theory, but without a real option to do a synchronous HTTP request present in node core, in practice it doesn't work.

Thanks. But in practice, I think it will. Let's discuss further.

connorjclark · 2019-08-21T22:05:55Z

Bumping b/c I believe I addressed your concerns in my previous comment, and believe this to be a very useful feature.

connorjclark · 2019-10-04T22:31:25Z

gentle ping

connorjclark · 2020-08-01T21:15:30Z

Please consider this patch again and my comments addressing your concerns.

connorjclark added 4 commits March 2, 2019 13:59

sourceMap.content auto - resolve input source maps with minimal confi…

3905ead

…guration

fix failing test

4fb781c

throw error if stderr exists, to debug why windows fails in travis

eb56dde

fix command

61f068e

L2jLiga reviewed Mar 4, 2019

View reviewed changes

connorjclark added 2 commits March 3, 2019 21:06

pr changes

09b25f2

use http module to get source contents

0a61ef7

connorjclark commented Mar 4, 2019

View reviewed changes

ditch async await for now

e21dc0a

connorjclark commented Mar 4, 2019

View reviewed changes

fix process exit

ff52308

connorjclark closed this Mar 15, 2019

connorjclark reopened this Mar 15, 2019

remove istanbul from test script

b9e7bb0

Merge remote-tracking branch 'upstream/master' into sourcemap-auto

37c0bad

connorjclark added 3 commits March 18, 2019 09:59

restore package lock

0b4f2f0

restore tests deleted by mistake

b73de80

remove log statement

cbd278d

fabiosantoscode closed this Apr 14, 2019

connorjclark mentioned this pull request May 24, 2019

Webpack is producing invalid sourcemaps? webpack/webpack#8302

Closed

connorjclark mentioned this pull request Jun 9, 2019

Support multiple --in-source-map arguments mishoo/UglifyJS#145

Closed

sourceMap.content "auto" option #298

sourceMap.content "auto" option #298

Conversation

connorjclark commented Mar 2, 2019 • edited

overview

other stuff

L2jLiga left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

connorjclark commented Mar 4, 2019

connorjclark commented Mar 4, 2019 • edited

L2jLiga commented Mar 4, 2019

connorjclark commented Mar 4, 2019 • edited

connorjclark commented Mar 4, 2019

Choose a reason for hiding this comment

connorjclark commented Mar 6, 2019

L2jLiga commented Mar 7, 2019

fabiosantoscode commented Mar 10, 2019

fabiosantoscode commented Mar 10, 2019

connorjclark commented Mar 10, 2019

connorjclark commented Mar 15, 2019

connorjclark commented Mar 15, 2019

L2jLiga commented Mar 15, 2019

L2jLiga commented Mar 17, 2019

connorjclark commented Mar 18, 2019

connorjclark commented Mar 21, 2019

fabiosantoscode commented Apr 14, 2019

connorjclark commented Apr 15, 2019 • edited

connorjclark commented Aug 21, 2019

connorjclark commented Oct 4, 2019

connorjclark commented Aug 1, 2020

connorjclark commented Mar 2, 2019 •

edited

connorjclark commented Mar 4, 2019 •

edited

connorjclark commented Mar 4, 2019 •

edited

connorjclark commented Apr 15, 2019 •

edited