refactor: build scripts before release

[mbrevda]

Fixes #5764: build lib before packing. This utilizes the npm prepack hook to build the scripts prior to packing:

➜  npm pack

> uglify-js@3.17.4 prepack
> bin/uglifyjs --self --beautify --source-map --output index.js

npm notice
npm notice 📦  uglify-js@3.17.4
npm notice === Tarball Contents ===
npm notice 1.3kB  LICENSE
npm notice 57.7kB README.md
npm notice 24.3kB bin/uglifyjs
npm notice 1.0MB  index.js
npm notice 1.1kB  package.json
npm notice 794B   tools/tty.js
npm notice === Tarball Details ===
npm notice name:          uglify-js
npm notice version:       3.17.4
npm notice filename:      uglify-js-3.17.4.tgz
npm notice package size:  175.7 kB
npm notice unpacked size: 1.1 MB
npm notice shasum:        1ad7ab4fda8f1944e9d4d2d125634cb312fdbdf5
npm notice integrity:     sha512-fIql/3fvUlF8f[...]bFgYARTUWpgVA==
npm notice total files:   6
npm notice
uglify-js-3.17.4.tgz

That produces the following tarball:

➜   tar tzfv uglify-js-3.17.4.tgz
-rw-r--r--  0 0      0        1348 Oct 26  1985 package/LICENSE
-rwxr-xr-x  0 0      0       24277 Oct 26  1985 package/bin/uglifyjs
-rw-r--r--  0 0      0     1038839 Oct 26  1985 package/index.js
-rw-r--r--  0 0      0         794 Oct 26  1985 package/tools/tty.js
-rw-r--r--  0 0      0        1144 Oct 26  1985 package/package.json
-rw-r--r--  0 0      0       57722 Oct 26  1985 package/README.md

[alexlamsl]

Note that bin/uglifyjs does require() files from tools/:

UglifyJS/bin/uglifyjs

Lines 6 to 11 in f2b6f1d

	require("../tools/tty");
	var fs = require("fs");
	var info = require("../package.json");
	var path = require("path");
	var UglifyJS = require("../tools/node");

The first one you'll need to include with the npm package (it contains Node.js specific workarounds for TTY across all versions and OSes), while the second one you should be able to get away with require("..") which should make it look up main field inside package.json instead.

[alexlamsl]

To clarify, the index.js you generate contains all the API side of uglify-js, whereas bin/uglifyjs utilises said API to provide a command-line tool for other users, i.e. bundlers should not be including the latter.

[mbrevda]

Pushed the changes - kindly point out if I missed anything!

[mbrevda]

Please note that require("..") only works after the initial build. This means that you can only use the cli to build if you've already built...

edit: one possible solution would be to include an index.js which required tools/node.js by default but would be overwritten at build time (shouldn't really be visible to anyone, especially if building/releasing via CI/CD)

[alexlamsl]

one possible solution would be to include an index.js which required tools/node.js by default but would be overwritten at build time

sounds good 👍

[mbrevda]

Changes pushed

[alexlamsl]

LGTM − let's give those CI tests a spin

[alexlamsl]

I've fixed a handful of issues, but there are remaining ones. Basically run the following:

$ npm pack

> uglify-js@3.17.4 prepack
> node bin/uglifyjs --self --no-wrap --beautify --source-map --output index.js

uglify-js-3.17.4.tgz

$ npm test
<should all passes, but I see 5 mocha test failures>

As node test/compress currently passes, when you debug it is faster to just skip those and run node test/mocha directly.

[mbrevda]

I think the tests are (mostly) failing as they expect a CJS module, not a global-poluting iife. Adding module.exports = UglifyJS; to built code brings the number of failed tests down to just 5; does Uglify have a way to generate a CJS module?

Also, do you want the CI tests to be updated to test the built code? That would allow PRs to fail in a case like this PR

[alexlamsl]

I think the tests are (mostly) failing as they expect a CJS module, not a global-poluting iife.

That is due to the wrong (inferred) --wrap option, which I've fixed in a6313e0 (you will need git pull to update your local copy) so no need to add module.exports = UglifyJS;

Also, do you want the CI tests to be updated to test the built code?

Good idea − I think this can be done with npm pack (or npm prepack if we want to skip the .tar.gz file) after npm install:

UglifyJS/test/release/install.sh

Lines 48 to 60 in f2b6f1d

	nvs use $NODE
	node --version
	npm config set audit false
	npm config set fund false
	npm config set loglevel error
	npm config set optional false
	npm config set save false
	npm config set strict-ssl false
	npm config set update-notifier false
	npm --version
	while !(npm install); do
	while !(npm cache clean --force); do echo "'npm cache clean' failed - retrying..."; done
	done

[mbrevda]

I'm not seeing the tests failing locally after rebasing. I've added the build step to the test setup script - can you kindly approve workflows so that the tests can run?

[mbrevda]

nm - I see them now. Working on it

[alexlamsl]

Will run the CI tests anyway, since we want to make sure the tests do actually fail here too 😉

[mbrevda]

The tests failed alright, but I'm stumped on how to fix them...

1. 2 falures pertain to bug reports - not really sure how that's all setup but guessing there might be something wrong with the built script
2. another issue is when "build when using --self with spidermonkey" - might that be related to the change in a6313e0?
3. The strangest of all is "Should work with mangle.properties.regex from --config-file" - cant see how this PR would affect that (and don't know the internals enough to know where to start digging)

[alexlamsl]

I think these issues are all caused by the bits we are dropping away:

• helper functions inside tools/node.js
• --self actually go and read those source files under lib/, and the list is provided by tools/node.js
• tools/domprops.json (parsed by tools/node.js, dropped by --self since the result would be too large)

[mbrevda]

Ok, so what do you recommend?

[alexlamsl]

So I think after generating the packaged index.js, you will need to:

• prepend to include domprops.json like so:

  var domprops = require("./tools/domprops.json");

  (obviously need to add it to package.json as well)
• prepend the FILES variable (probably read from runtime, then relative() out the current working directory or something):
  

  UglifyJS/tools/node.js

  Lines 3 to 16 in f2b6f1d

  	exports.FILES = [
  	require.resolve("../lib/utils.js"),
  	require.resolve("../lib/ast.js"),
  	require.resolve("../lib/transform.js"),
  	require.resolve("../lib/parse.js"),
  	require.resolve("../lib/scope.js"),
  	require.resolve("../lib/compress.js"),
  	require.resolve("../lib/output.js"),
  	require.resolve("../lib/sourcemap.js"),
  	require.resolve("../lib/mozilla-ast.js"),
  	require.resolve("../lib/propmangle.js"),
  	require.resolve("../lib/minify.js"),
  	require.resolve("./exports.js"),
  	];

• append the following section from tools/node.js:
  

  UglifyJS/tools/node.js

  Lines 26 to 110 in f2b6f1d

  	function to_comment(value) {
  	if (typeof value != "string") value = JSON.stringify(value, function(key, value) {
  	return typeof value == "function" ? "<[ " + value + " ]>" : value;
  	}, 2);
  	return "// " + value.replace(/\n/g, "\n// ");
  	}
  	if (+process.env["UGLIFY_BUG_REPORT"]) exports.minify = function(files, options) {
  	if (typeof options == "undefined") options = "<<undefined>>";
  	var code = [
  	"// UGLIFY_BUG_REPORT",
  	to_comment(options),
  	];
  	if (typeof files == "string") {
  	code.push("");
  	code.push("//-------------------------------------------------------------")
  	code.push("// INPUT CODE", files);
  	} else for (var name in files) {
  	code.push("");
  	code.push("//-------------------------------------------------------------")
  	code.push(to_comment(name), files[name]);
  	}
  	if (options.sourceMap && options.sourceMap.url) {
  	code.push("");
  	code.push("//# sourceMappingURL=data:application/json;charset=utf-8;base64,eyJ2ZXJzaW9uIjozLCJzb3VyY2VzIjpbXSwibmFtZXMiOltdLCJtYXBwaW5ncyI6IiJ9");
  	}
  	var result = { code: code.join("\n") };
  	if (options.sourceMap) result.map = '{"version":3,"sources":[],"names":[],"mappings":""}';
  	return result;
  	};
  	function describe_ast() {
  	var out = OutputStream({ beautify: true });
  	doitem(AST_Node);
  	return out.get() + "\n";
  	function doitem(ctor) {
  	out.print("AST_" + ctor.TYPE);
  	var props = ctor.SELF_PROPS.filter(function(prop) {
  	return !/^\$/.test(prop);
  	});
  	if (props.length > 0) {
  	out.space();
  	out.with_parens(function() {
  	props.forEach(function(prop, i) {
  	if (i) out.space();
  	out.print(prop);
  	});
  	});
  	}
  	if (ctor.documentation) {
  	out.space();
  	out.print_string(ctor.documentation);
  	}
  	if (ctor.SUBCLASSES.length > 0) {
  	out.space();
  	out.with_block(function() {
  	ctor.SUBCLASSES.sort(function(a, b) {
  	return a.TYPE < b.TYPE ? -1 : 1;
  	}).forEach(function(ctor, i) {
  	out.indent();
  	doitem(ctor);
  	out.newline();
  	});
  	});
  	}
  	}
  	}
  	function infer_options(options) {
  	var result = exports.minify("", options);
  	return result.error && result.error.defs;
  	}
  	exports.default_options = function() {
  	var defs = infer_options({ 0: 0 });
  	Object.keys(defs).forEach(function(component) {
  	var options = { module: false };
  	options[component] = { 0: 0 };
  	if (options = infer_options(options)) {
  	defs[component] = options;
  	}
  	});
  	return defs;
  	};

[mbrevda]

Why isn't to_comment being included? Is it being marked internally as pure? Is there a way to opt out of DCE for a single function?

[alexlamsl]

Why isn't to_comment being included?

Because the file tools/node.js is not included − would have caused problems since it is reading and importing files which we are trying not to rely on 😉

--self concatnates list of FILES as quoted above (with a thin wrapper into a global variable named UglifyJS by default, but we have disabled that in this case).

[mbrevda]

It seems the rest of this PR is better dealt with by someone with greater knowledge of Uglify Internals. Glad to have gotten it this far, and I am looking forward to seeing it merged!