Problematic "fromString" option choices

[quaderi]

See exports.minify in tools/node.js:

            var code = options.fromString
                ? file
                : fs.readFileSync(file, "utf8");
            sourcesContent[file] = code;
            toplevel = UglifyJS.parse(code, {
                filename: options.fromString ? "?" : file,

1. if options.fromString is true (or truthy), the second line here is essentially the same as:

            sourcesContent[code] = code;

I can't imagine this was really the intent... "code" can be BIG.

2. if options.fromString is true (or truthy), the "?" filename will end up in "sources" in the sourcemap string.

This is likely to cause problems for whoever is consuming the output. A main reason someone would be using the fromString option in the first place is probably because they are performing a series of data transforms in memory (through say something like gulp) and don't want to keep rewriting and rereading the data to a temporary file on disk just to transform it again. They may very well have a filename they could provide here -- they just can't have UglifyJS reading that filename from disk at this point (for performance or some other reason). There's currently really no nice way to override the "?" in these cases. Perhaps the best (still hacky and fragile) way to do that currently is to post-process the output, parsing the JSON, modifying sources and stringifying again -- that can cause a significant performance hit for files with large sourcemaps and almost no one understands the bugs and the library well enough to get this far in fixing the issue anyway.

Thankfully, the problem of virtual files (with names) for Node is solved by wearefractal's popular (and relatively concise) vinyl library. It's the defacto standard across things like gulp and stores content in a way that's very simple to interact with. Using a variant of fromString design for virtual files, I think it'll be relatively easy to close a number of issues (#263, #576). There are also a lot of downstream issues this would make resolvable: e.g. terinjokes/gulp-uglify#56, dustinspecker/generator-ng-poly#38

3. If options.fromString is false (or falsy), the code is using fs.readFileSync -- the synchronous, i.e. blocking method -- instead of readFile. This makes using it a dangerous choice in web servers.

The core idea behind node is to leverage asynchronous non-blocking code for performance. Node is single-threaded meaning the entire node process has to sit around doing nothing while the blocking readFileSync executes (an expensive IO operation as it hits the disk and reads the whole files). There are rarely any good reasons to use readFileSync, and in this case, as far as I can tell there really, really isn't one. The async library is already a dependency and makes it pretty easy to do this for something like an array of files -- so why not use it? (note: some code will have to be refactored into a callback function, but this is well worth the effort for such a serious server performance killer.) The readFileSync issue comes up again with reading the input source map (options.inSourceMap). If there's some concern about "callback hell" from asynchronous js, modularization helps a lot, but there also great promises libraries out there (e.g. bluebird) that can be used to write solid asynchronous js for node which is still readable if the async library isn't cutting it.

[eskatos]

Good thing that this is being tackled!

[quaderi]

For anyone interested, I ended up forking UglifyJS2. My fork is UglyflyJS and it fixes the issues mentioned above among other things. You can read its README for more info. Because it requires an async API (version breaking change) to fix the third issue I'm not submitting the changes as a pull request. However looking at its implementation of dealing with input files might be useful in addressing the problems with the fromString options here.