fix: minified versionLt did not match the original function

[mpeyper]

I found an issue with module federation and the strictVersion option of shared packages. I have detailed my findings here, but the short version is the minified versionLt function is not comparing semver version correctly, but when using the original version in NodeJS, it does.

My investigation found that the minified version did not match the original function, so this PR is to correct that.

What kind of change does this PR introduce?

Bug Fix

Did you add tests for your changes?

No. There are already tests around the versionLt function, but the minified templates do not have any tests run against them. If this is an issue, please advise how you want them tested and I can look into it.

Does this PR introduce a breaking change?

No.

What needs to be documented once your changes are merged?

Nothing should need to be added.

[jsf-clabot]

Thank you for your submission, we really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.

[webpack-bot]

For maintainers only:

• This needs to be documented (issue in webpack/webpack.js.org will be filed when merged)
• This needs to be backported to webpack 4 (issue will be created when merged)

[mpeyper]

I signed the CLA with the email it defaulted to (my personal email), but the commit was under my work email. Will that cause issue with the CLA check?

[snitin315]

Yes

[mpeyper]

@snitin315 do you know how I fix it? It doesn't let me change the email address now. I could force push the commit with a different author email (I think)?

[mpeyper]

Preempting the testing thing, I went ahead and tried to write something to test the runtime code. I've managed to get this working:

describe("versionLt", () => {
		const cases = [
			"1 < 2",
			"99 < 100",
			// ... more cases
		];
		for (const c of cases) {
			const parts = c.split(" < ");
			const a = parts[0];
			const b = parts[1];

			const runtimeFunction = versionLtRuntimeCode({
				basicFunction(args, body) {
					return `(${args}) => {\n${body.join('\n')}\n}`;
				}
			});

			const versionLtRuntime = eval(`(function (...args) { ${runtimeFunction}; return versionLt(...args); })`)

			it(c, () => {
				expect(versionLt(a, a)).toBe(false);
				expect(versionLt(b, b)).toBe(false);
				expect(versionLt(a, b)).toBe(true);
				expect(versionLt(b, a)).toBe(false);
			});
	
			it(`${c} (runtime)`, () => {
				expect(versionLtRuntime(a, a)).toBe(false);
				expect(versionLtRuntime(b, b)).toBe(false);
				expect(versionLtRuntime(a, b)).toBe(true);
				expect(versionLtRuntime(b, a)).toBe(false);
			});
		}
	});

Obviously using eval is a bit gross, but when I tried to use new Function(...) it threw an error that parseVersion didn't exist and I found a note at the bottom an MSDN example that says it won't work in NodeJS environments 🤷‍♂️ .

If you want these, I can add them to the PR, otherwise I'm happy to leave it untested. I think ultimately it would be awesome if the minified versions could be generated from the original instead of hand rolled, but that's a bit beyond my understanding of this codebase at this point.

[snitin315]

@snitin315 do you know how I fix it? It doesn't let me change the email address now. I could force push the commit with a different author email (I think)?

Yes you can force push.

[sokra]

Hmm this really bother me. I throught this could happen and we would need some kind of linting step to verify that it's minimized correctly, but I have never implemented that.

Thanks for pointing out that issue and I'll take it as opportunity to implement a linting step that does the minimizing and validates it.

[mpeyper]

@sokra, just to clarify, are you saying you won't take this change and pursue a better fix or will you take this for now (if I fix the CLA issue and/or add the test above) and work towards the better fix in time?

[mpeyper]

I had some spare time tonight, so I tried to write a thing to minify the original functions when building the templates so that it could never get out of sync. I ended up with:

const { minify } = require('terser');

const makeMinified = (runtimeTemplate, fn, args) => {
	const minifiedFn = minify(`const fn = ${fn.toString()}`, { mangle: { reserved: args } }).code;
	const fnBody = minifiedFn.replace(`const fn=(${args.join(",")})=>{`, "").slice(0, -2);
	return `var ${fn.name} = ${runtimeTemplate.basicFunction(args.join(", "), [
		"// see webpack/lib/util/semver.js for original code",
		fnBody
	])}`;
};

The could then be called like:

const versionLt = (a, b) => {
	// ...
};
/* eslint-enable eqeqeq */
exports.versionLt = versionLt;


exports.versionLtRuntimeCode = runtimeTemplate => makeMinified(runtimeTemplate, versionLt, ["a", "b"]);

This worked for that tests I previously posted, but as soon as I tried to apply it to parseVersion it fell apart because the way the minified version is constructed is completely different (I should have checked the other before starting). I'll concede that extracting the funBody from the terser output is a bit gross with it's magic templates and slice indexes, but I was struggling to think of a way around it given how runtimeTemplate.basicFunction works.

I'm done for the night, but let me know if this idea has legs and I'll try to expand upon it in the morning.

[mpeyper]

@sokra It looks like your commit is doing something similar to what I tried, only better and more complete but at build time instead of runtime.

[mpeyper]

@sokra For what it's worth, I continued to pluck away at building the minified version at runtime, mostly or my own learning, and not have all of them producing minified output that passes the same tests the original versions do. The trick came from your change to move splitAndConvert into the parseVersion and parseRange functions, then the same generation code could be used for all of the functions. I also had to play some silly buggers with the number of argument and parenthesis.

const { minify } = require('terser');

const makeMinified = (runtimeTemplate, fn, args) => {
	const minifiedFn = minify(`const fn = ${fn.toString()}`, { mangle: { reserved: args } }).code;
	const fnBody = minifiedFn.replace(`const fn=${args.length === 1 ? args[0] : `(${args.join(",")})`}=>{`, "").slice(0, -2);
	return `var ${fn.name} = ${runtimeTemplate.basicFunction(args.join(", "), [
		"// see webpack/lib/util/semver.js for original code",
		fnBody
	])}`;
};

// original functions...

exports.parseVersionRuntimeCode = runtimeTemplate => makeMinified(runtimeTemplate, parseVersion, ["str"]);
exports.versionLtRuntimeCode = runtimeTemplate => makeMinified(runtimeTemplate, versionLt, ["a", "b"]);
exports.rangeToStringRuntimeCode = runtimeTemplate => makeMinified(runtimeTemplate, rangeToString, ["range"]);
exports.satisfyRuntimeCode = runtimeTemplate => makeMinified(runtimeTemplate, satisfy, ["range", "version"]);

Out of curiosity, do you feel like the build time approach you are taking has any major benefits over runtime generation?

Are you interested in the tests I have that construct and evaluate the runtime versions to run the test cases against? I can add them on top of your commit after you get the build green if you want them.

[webpack-bot]

Thank you for your pull request! The most important CI builds succeeded, we’ll review the pull request soon.

[sokra]

Yes tests are always good. Maybe in a separate PR as I want to publish this bugfix as soon as possible.

I did go for the lint time approach because I didn't want to introduce terser as non-dev dependency and also save the time for spinning up terser and minimizing during webpack runtime. So mostly a performance choice.

[mpeyper]

That makes sense. I'll put together a PR for the tests soon.

[mpeyper]

New PR has been raised for the tests (#11280).

@sokra, any idea when this might hit v5.0.0-beta.24?

[sokra]

Today