[WIP][3.x] Make minify portable by making it work without file system by avdg · Pull Request #1366 · mishoo/UglifyJS

avdg · 2016-11-05T23:11:50Z

Current status: Preparing for merge. Open for feedback.

TODO:

Give functions custom errors under same new error type
Fix regression
Look into extra tests for name caching
Make readNameCache and writeNameCache internal api, relying on readFile and writeFile as platform depending "drivers"
Documentate writeFile and readFile as overwritable functions in portable version. (Not so for simple_glob, which will be unofficial api even though it is as easy to overwrite this function)
Make the api work without simple_glob being available.
Add tests for portable UglifyJS version
Test errors
~~Test readFile individually~~ (should be covered enough?)

EXTRA:

Hmmm.. what about async...

avdg · 2016-11-06T00:01:30Z

Demo:

Open https://avdg.github.io/uglify/#ref_PR_x2d1366
Open console
Type code, UglifyJS is the uglify object.

Example code:

Basic example: UglifyJS.minify("var foo = bar", {fromString: "true"})
Source maps: UglifyJS.minify("var foo = function(a, b) { return a + b; }", {fromString: "true", sourceMapUrl: false, outSourceMap: "out.js.map"})

kzc · 2016-11-06T19:44:35Z

lib/minify.js

@@ -0,0 +1,125 @@
+exports.readFile = exports.simple_glob = exports.readNameCache =
+    exports.writeNameCache = function() {
+        throw Error("UglifyJS has no access to the file system");


Would prefer a top level named error function followed by four lines with one assignment per line.

kzc · 2016-11-06T19:46:09Z

lib/minify.js

+    if (options.spidermonkey) {
+        toplevel = AST_Node.from_mozilla_ast(files);
+    } else {
+        function addFile(file, fileUrl) {


Can't declare a function in an if or else:

$ npm test

uglify-js@2.7.4 test /home/travis/build/mishoo/UglifyJS2
node test/run-tests.js
undefined:9153
function addFile(file, fileUrl) {
^^^^^^^^
SyntaxError: In strict mode code, functions can only be declared at top level or immediately within another function.

Actually, this is the same code (only moved a bit). And didn't fix (although I could) because I spend time doing other things apparently.

from: https://travis-ci.org/mishoo/UglifyJS2/builds/173580473

related: #1052, #1053

This should be fixed

kzc · 2016-11-06T19:53:20Z

Assuming all tests pass, the patch looks fine.

Although not much code was changed in minify(), the mangle name caches may not have been tested extensively in the previously existing minify() tests.

Is it possible for users to override the four minify functions that use the file system to provide their own browser equivalents of a fake file system if they wanted?

avdg · 2016-11-06T21:16:58Z

Is it possible for users to override the four minify functions that use the file system to provide their own browser equivalents of a fake file system if they wanted?

Yes, export is the same object as UglifyJS. As long as users can reach that code, users can change the functionality.

avdg · 2016-11-06T23:40:42Z

I might remove readNameCache and writeNameCache as drivers (like we done it) and make it internal api. readNameCache depends on reading a file, which can be done by the readFile "driver" (note the quotes) and I'll add a writeFile "driver" fro writeNameCache.

avdg · 2016-11-06T23:42:52Z

Hmm I have to see, maybe we should allow external api to pass data to these read/write nameCache properties in some way (without changing other api's). I have to see how to refactor this yet.

avdg · 2016-11-06T23:45:13Z

I won't make simple_glob platform independent for now though... Maybe I can change the code so 'globbing' would only be done when the function is available.

kzc · 2016-11-07T00:54:22Z

There's no files to glob in the browser (no equivalent of fs.readdirSync), so a no op is fine.

kzc · 2016-11-07T07:09:02Z

To abstract the file system, hooks would be needed for these cases:

$ egrep '(fs|path)\.' tools/node.js 
    return fs.realpathSync(path.join(path.dirname(__filename), file));
    return fs.readFileSync(file, "utf8");
                : fs.readFileSync(file, "utf8");
        inMap = JSON.parse(fs.readFileSync(options.inSourceMap, "utf8"));
    var data = fs.readFileSync(filename, "utf8");
    return readReservedFile(path.join(__dirname, "domprops.json"), reserved);
            var cache = fs.readFileSync(filename, "utf8");
            data = fs.readFileSync(filename, "utf8");
        fs.writeFileSync(filename, JSON.stringify(data, null, 2), "utf8");
        var dir = path.dirname(glob);
            var entries = fs.readdirSync(dir);
            var pattern = "^" + (path.basename(glob)

They needn't be implemented for the browser.

avdg · 2016-11-07T13:38:55Z

readNameCache and writeNameCache won't be part of the official api unless given some thought (maybe an api for name cache without fs access is doable), although available for convenience for the still unadopted bin/uglify that uses them.

avdg · 2016-12-19T16:35:21Z

Going to move the current progress to harmony...avdg:portable-minify-backup

Then rebase this patch to be mergeable with master again.

kzc · 2016-12-20T14:31:36Z

lib/minify.js

@@ -0,0 +1,168 @@
+exports.readFile = function() {
+    DefaultsError.croak("UglifyJS has no access to read from the file system");


Error message is too verbose - "readFile not supported" is sufficient. Ditto for writeFile and simple_glob.

kzc · 2016-12-20T17:29:22Z

You could add a test in test/mocha/cli.js for the --self version of minify() with fromString: true.

avdg · 2016-12-20T17:39:42Z

Ow yeah, these tests there generate the portable version.

avdg · 2016-12-24T23:49:36Z

Added few tests on the hooks (and fromString). There are still some additional tests needed for readNameCache, writeNameCache and the errors (as it seems that the errors simply don't show up).

kzc · 2016-12-25T00:03:47Z

lib/minify.js

+};
+
+exports.simple_glob = function(files) {
+    return files;


Should return an array of strings if a scalar is passed:

exports.simple_glob = function(files) { return Array.isArray(files) ? files : [ files ]; }

as per comments in the original implementation:

// Argument `glob` may be a string or an array of strings. // Returns an array of strings. Garbage in, garbage out.

Seems the current code handles string as parameter fine, adding test as extra insurance for now.

concat handles this flexible, non-arrays are implicitly treated as an array with 1 element.

https://github.com/mishoo/UglifyJS2/pull/1366/files/0750f4a338cfdc557d96fc68382751c0b23d873a#diff-29c0119b6a96bae34223c40f1c7f774bL79

kzc · 2016-12-25T00:13:30Z

test/mocha/portable.js

+    });
+
+    it("Should minify from a string successfully", function() {
+        assert.strictEqual(minifier.minify('console.log("Hello world!");', {fromString: true}).code,


Suggest minifying 'console["log"]("Hello " + "world!");' in all your examples in this file to show that minify() actually did something.

avdg · 2016-12-25T16:46:01Z

I think I'm renaming simple_glob to glob just because the glob doesn't have to be simple.

kzc · 2016-12-25T17:02:24Z

I think I'm renaming simple_glob to glob just because the glob doesn't have to be simple.

It's already part of the external API and shouldn't be renamed.

avdg · 2016-12-25T17:09:38Z

I think I'm renaming simple_glob to glob just because the glob doesn't have to be simple.

It's already part of the external API and shouldn't be renamed.

That's unfortunate then...

avdg · 2016-12-26T01:34:56Z

About the renaming, maybe it might be listed in a separate issue, ready to fill any gap of opportunity. Given that minifiers are getting competition and that this is a relative small task that leads to better consistency of the api this might be an alternative option.

This will also give more reasons to keep simple_glob part of the unofficial api, which is already an upgrade from being undocumented.

kzc · 2016-12-26T06:20:43Z

Please keep the external API the same. You can add to it, but not remove stuff from it.

Remember that uses_arguments was also undocumented but used in the wild: #1299

kzc · 2016-12-26T06:57:13Z

Given that minifiers are getting competition and that this is a relative small task that leads to better consistency of the api this might be an alternative option.

If there's an UglifyJS2 3.x release then the external API can be altered. Probably the same time as minify() is dropped into bin/uglifyjs and/or harmony is merged into master - after the ES6 scope and compress bugs are fixed.

Another thing that should be done in a hypothetical 3.x release is to flatten the overly complicated uglify options that currently has 4 or 5 different sub-sections: mangle, compress, mangleProps, and output (a.k.a. beautify) - and unify the options between bin/uglify and minify().

avdg

Don't set options.mangleProperties.cache if options.mangleProperties is false (must convert to an object like value first)

avdg · 2016-12-29T14:12:05Z

lib/minify.js

+
+    // 3. mangle properties
+    if (options.mangleProperties || options.nameCache) {
+        options.mangleProperties.cache = readNameCache(options.nameCache, "props");


options.mangleProperties defaults to false, which isn't an object :/ Need to know how this stuff is working before I can fix this...

This code seems to come from #910 and isn't actually tested properly

Just using default value to fix this...

avdg · 2017-04-10T13:14:39Z

I really hope I didn't do anything wrong and that the test coverage is good. It's easy to lose track of changes because lib/minify.js file is maintained without an easy way to keep it in sync.

avdg · 2017-04-10T13:19:52Z

Gonna attempt to stage it as is, additional features can be added and there is nothing structural anyway.

avdg · 2017-04-10T13:22:30Z

Did a manual diff of the minify function, should look good

alexlamsl · 2017-04-10T13:56:28Z

Much better - only Travis CI is upset now:

https://travis-ci.org/mishoo/UglifyJS2/jobs/220551459#L244-L247

I think we need to pull addFile() out of the else block:

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

avdg · 2017-04-10T14:11:41Z

oh yeah, it should be done differently, easy peasy...

avdg · 2017-04-10T14:14:14Z

well, moving outside the else block shouldn't be enough... I defined it as an inline function.

alexlamsl · 2017-04-10T14:21:32Z

tools/exports.js

 exports["SymbolDef"] = SymbolDef;
+exports["minify"] = minify;
+exports["readNameCache"] = readNameCache;
+exports["writeNameCache"] = writeNameCache;


Let's not export readNameCache & writeNameCache, as we never do before anyway.

bin/uglifyjs depends on it

we might do a deprecation policy

Ah yes, it is exported directly under tools/node.js prior to this PR.

avdg · 2017-04-10T14:29:45Z

Just removed a whitespace

alexlamsl · 2017-04-10T15:27:03Z

Please update #1366 (diff) to use UglifyJS.base64Decoder

avdg · 2017-04-10T16:31:51Z

Done so for UglifyJS.base64Decoder, implemented UglifyJS.base64Encoder as well.

kzc · 2017-04-10T19:10:27Z

lib/minify.js

+};
+
+var defaultBase64Decoder = exports.base64Decoder = function(input) {
+    DefaultsError.croak("No base64 decoder implemented");


Should fallback to using atob here if typeof atob == "function".

kzc · 2017-04-10T19:10:53Z

lib/minify.js

+}
+
+var defaultBase64Encoder = exports.base64Encoder = function(input) {
+    DefaultsError.croak("No base64 encoder implemented");


Should fallback to using btoa here if typeof btoa == "function".

avdg · 2017-04-10T19:58:22Z

Add default fallback for web environment
Add tests

- refactor `screw_ie8` to `ie8` - compact `sourceMap` options - more stringent verification on input `options` - toplevel shorthands - `ie8` - `keep_fnames` - `toplevel` closes mishoo#96 closes mishoo#1366 fixes mishoo#124 fixes mishoo#263 fixes mishoo#379 fixes mishoo#423 fixes mishoo#576 fixes mishoo#737 fixes mishoo#958 fixes mishoo#1036 fixes mishoo#1175 fixes mishoo#1220 fixes mishoo#1223 fixes mishoo#1280

- refactor `screw_ie8` to `ie8` - compact `sourceMap` options - more stringent verification on input `options` - toplevel shorthands - `ie8` - `keep_fnames` - `toplevel` - deprecated `fromString` in `minify()` - `minify()` no longer handles any `fs` operations - unify order of operations for `mangle_properties()` on CLI & API - `bin/uglifyjs` used to `mangle_properties()` before even `Compressor` - `minify()` used to `mangle_properties()` after `Compressor` but before `mangle_names()` - both will now do `Compressor`, `mangle_names()` then `mangle_properties()` closes mishoo#96 closes mishoo#1366 fixes mishoo#124 fixes mishoo#263 fixes mishoo#379 fixes mishoo#423 fixes mishoo#576 fixes mishoo#737 fixes mishoo#958 fixes mishoo#1036 fixes mishoo#1175 fixes mishoo#1220 fixes mishoo#1223 fixes mishoo#1280

- refactor `screw_ie8` to `ie8` - compact `sourceMap` options - more stringent verification on input `options` - toplevel shorthands - `ie8` - `keep_fnames` - `toplevel` - deprecated `fromString` in `minify()` - `minify()` no longer handles any `fs` operations - unify order of operations for `mangle_properties()` on CLI & API - `bin/uglifyjs` used to `mangle_properties()` before even `Compressor` - `minify()` used to `mangle_properties()` after `Compressor` but before `mangle_names()` - both will now do `Compressor`, `mangle_names()` then `mangle_properties()` - `options.parse` / `--parse` for parser options beyond `bare_returns` closes mishoo#96 closes mishoo#1366 fixes mishoo#124 fixes mishoo#263 fixes mishoo#379 fixes mishoo#423 fixes mishoo#576 fixes mishoo#737 fixes mishoo#958 fixes mishoo#1036 fixes mishoo#1175 fixes mishoo#1220 fixes mishoo#1223 fixes mishoo#1280

avdg · 2017-04-14T07:30:06Z

I'm quite busy for now as I do lots of interviews, so here my apology that I even can't do simple fixes that requires more than 1 click.

alexlamsl · 2017-04-14T07:51:49Z

@avdg no worries - I think #1811 will eventually take over 👍

- rename `screw_ie8` to `ie8` - rename `mangle.except` to `mangle.reserved` - rename `mangle.properties.ignore_quoted` to `mangle.properties.keep_quoted` - compact `sourceMap` options - more stringent verification on input `options` - toplevel shorthands - `ie8` - `keep_fnames` - `toplevel` - `warnings` - support arrays and unquoted string values on CLI - drop `fromString` from `minify()` - `minify()` no longer handles any `fs` operations - unify order of operations for `mangle_properties()` on CLI & API - `bin/uglifyjs` used to `mangle_properties()` before even `Compressor` - `minify()` used to `mangle_properties()` after `Compressor` but before `mangle_names()` - both will now do `Compressor`, `mangle_names()` then `mangle_properties()` - `options.parse` / `--parse` for parser options beyond `bare_returns` - add `mangle.properties.builtins` to disable built-in reserved list - disable with `--mangle-props builtins` on CLI - `warnings` now off by default - add `--warn` and `--verbose` on CLI - drop `--enclose` - drop `--export-all` - drop `--reserved-file` - use `--mangle reserved` instead - drop `--reserve-domprops` - enabled by default, disable with `--mangle-props domprops` - drop `--prefix` - use `--source-map base` instead - drop `--lint` - remove `bin/extract-props.js` - limit exposure of internal APIs - update documentations closes #96 closes #102 closes #136 closes #166 closes #243 closes #254 closes #261 closes #311 closes #700 closes #748 closes #912 closes #1072 closes #1366 fixes #101 fixes #123 fixes #124 fixes #263 fixes #379 fixes #419 fixes #423 fixes #461 fixes #465 fixes #576 fixes #737 fixes #772 fixes #958 fixes #1036 fixes #1142 fixes #1175 fixes #1220 fixes #1223 fixes #1280 fixes #1359 fixes #1368

avdg · 2017-04-15T23:07:41Z

Hmm.. I might want to port the tests to the latest master

avdg mentioned this pull request Nov 5, 2016

UglifyJS.minify undefined in --self build #1223

Closed

kzc reviewed Nov 6, 2016

View reviewed changes

avdg mentioned this pull request Nov 14, 2016

Too slow when they are too many files #718

Closed

avdg force-pushed the portable-minify branch from 6694138 to 83369d5 Compare December 19, 2016 16:50

kzc reviewed Dec 20, 2016

View reviewed changes

avdg force-pushed the portable-minify branch from 30af788 to eb3250d Compare December 20, 2016 15:31

kzc reviewed Dec 25, 2016

View reviewed changes

avdg mentioned this pull request Dec 29, 2016

UglifyJS 3.0 changes (breaking changes only) #1411

Closed

avdg commented Dec 29, 2016

View reviewed changes

avdg changed the title ~~[WIP] Make minify portable by making it work without file system~~ Make minify portable by making it work without file system Apr 10, 2017

avdg force-pushed the portable-minify branch from 64b2900 to 3b01516 Compare April 10, 2017 14:14

alexlamsl reviewed Apr 10, 2017

View reviewed changes

avdg force-pushed the portable-minify branch from 3b01516 to 96f0793 Compare April 10, 2017 14:29

Make the minify api portable

9b16a75

avdg force-pushed the portable-minify branch from 96f0793 to 9b16a75 Compare April 10, 2017 16:30

kzc reviewed Apr 10, 2017

View reviewed changes

alexlamsl changed the title ~~Make minify portable by making it work without file system~~ [WIP][3.x] Make minify portable by making it work without file system Apr 11, 2017

alexlamsl mentioned this pull request Apr 13, 2017

unify CLI & API under minify() #1811

Merged

alexlamsl closed this in #1811 Apr 15, 2017

		@@ -0,0 +1,168 @@
		exports.readFile = function() {
		DefaultsError.croak("UglifyJS has no access to read from the file system");

Conversation

avdg commented Nov 5, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

avdg commented Nov 6, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

avdg Nov 6, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kzc Nov 6, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kzc commented Nov 6, 2016

Uh oh!

avdg commented Nov 6, 2016

Uh oh!

avdg commented Nov 6, 2016

Uh oh!

avdg commented Nov 6, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

avdg commented Nov 6, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kzc commented Nov 7, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kzc commented Nov 7, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

avdg commented Nov 7, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

avdg commented Dec 19, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kzc commented Dec 20, 2016

Uh oh!

avdg commented Dec 20, 2016

Uh oh!

avdg commented Dec 24, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kzc Dec 25, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

avdg commented Dec 25, 2016

Uh oh!

kzc commented Dec 25, 2016

Uh oh!

avdg commented Dec 25, 2016

Uh oh!

avdg commented Dec 26, 2016

Uh oh!

kzc commented Dec 26, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

avdg commented Nov 5, 2016 •

edited

Loading

avdg Nov 6, 2016 •

edited

Loading

kzc Nov 6, 2016 •

edited

Loading

avdg commented Nov 6, 2016 •

edited

Loading

avdg commented Nov 6, 2016 •

edited

Loading

kzc commented Nov 7, 2016 •

edited

Loading

kzc commented Nov 7, 2016 •

edited

Loading

avdg commented Nov 7, 2016 •

edited

Loading

avdg commented Dec 24, 2016 •

edited

Loading

kzc Dec 25, 2016 •

edited

Loading

kzc commented Dec 26, 2016 •

edited

Loading

avdg left a comment •

edited

Loading

avdg Apr 10, 2017 •

edited

Loading