avoid to change original options (.acorn) object

[LongTengDao]

This PR contains:

• bugfix
• feature
• refactor
• documentation
• other

Are tests included?

• yes (bugfixes and features will not be merged without tests)
• no

Breaking Changes?

• yes (breaking changes will not be merged unless absolutely necessary)
• no

List any relevant issue numbers: #3050

Description

I'm not sure how to test the side effects between multi bundling:

const acorn = {};

rollup({
  input: 'main.js',
  acorn,
  experimentalTopLevelAwait: true,
}).then(()=>{
  console.log(acorn.allowAwaitOutsideFunction);// true, then rollup below will work incorrectly
  rollup({
    input: 'main.js',
    acorn,
    experimentalTopLevelAwait: false,
  });
});

[codecov]

Codecov Report

Merging #3051 into master will not change coverage.
The diff coverage is 100%.

@@           Coverage Diff           @@
##           master    #3051   +/-   ##
=======================================
  Coverage   88.73%   88.73%           
=======================================
  Files         165      165           
  Lines        5733     5733           
  Branches     1748     1748           
=======================================
  Hits         5087     5087           
  Misses        388      388           
  Partials      258      258

Impacted Files	Coverage Δ
src/Graph.ts	92.98% <100%> (ø)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2593b04...3e13819. Read the comment docs.

[shellscape]

Thanks much for the extra info in #3050.

[lukastaegert]

I think it would be cool to have a general test that makes sure we do not mutate options. I would recommend to add a test to the misc/misc.js test file as the other sample based tests to some magic that may pollute the results.

If you look at the other tests in this file, you'll see it is rather straightforward. Maybe you could store the options in a variable and after the test do a deep equal with the options (and make sure the test is red without your modification). If you have more questions, just ask.

[LongTengDao]

@lukastaegert I added a simple test. I tried to do a general test, but I actually detected several changes of options in rollup, and I don't understand rollup enough to accomplish this huge task in this PR.

---

rollup/src/rollup/index.ts

Line 187 in 04b7d52

delete rawInputOptions.cache;

(Directly remove this line will cause checks fail, I can't figure why.)

---

rollup/src/rollup/index.ts

Line 100 in 04b7d52

plugin.name = `${ANONYMOUS_PLUGIN_PREFIX}${pluginIndex + 1}`;

(Maybe replace all plugin.name in the source to plugin.name || weakMap.get(plugin)?)

[lukastaegert]

How about making this test a little deeper by doing it like this:

	it('throw modification of options.acorn', () => {
		const plugins = [ loader({ input: `export default 0;` }) ];
		const options = {
				input: 'input',
				plugins,
				acorn: {},
				experimentalTopLevelAwait: true,
			};
		return rollup
			.rollup(options).then(() => assert.deepStrictEqual(options, {
				input: 'input',
				plugins,
				acorn: {},
				experimentalTopLevelAwait: true,
			});
	});

Or does this actually fail? That way, we would also check that other options are protected and we can easily extend the test to check more stuff.

[lukastaegert]

Ok, this might have issues with the plugins, but I think it is worth a try.

[LongTengDao]

The plugin returned by loader has no name field, and rollup will set a name for it. I can't remove all plugin.name modification in this PR, that's a big project. The easy way is add name in loader utils directly. Of cause, the easiest way is ignore plugin related modification.

But what I wonder is, since rollup is run in strict mode, just freeze the options, then if there is any modification to the options, an error will be thrown. Do you think there is any defect in this way?

[LongTengDao]

And assert.deepStrictEqual also can't promise no modification, if rollup set options' property value to another object (same structure), but not same object any more. Then if user had an refference to the original value object, and want to modify it, unexpected things will happen.

const acorn = { ecmaVersion: 6 };
const options = { acorn };
await rollup(options);// suppose rollup set options.acorn = Object.assign({}, options.acorn);
acorn.ecmaVersion = 7;
await rollup(options);// works unexpected

[lukastaegert]

just freeze the options, then if there is any modification to the options

You a re right, the thing with Object.freeze is that it only does a shallow freeze but not a "deep" freeze. Which is ok for this specific issue but would not work for checking the whole options object. But as you said since we are mutating plugins anyway, let's stick with what we have.

[LongTengDao]

Hi, you maybe miss my second message ;)

And assert.deepStrictEqual also can't promise no modification, if rollup set options' property value to another object (same structure), but not same object any more. Then if user had an refference to the original value object, and want to modify it, unexpected things will happen.

const acorn = { ecmaVersion: 6 };
const options = { acorn };
await rollup(options);// suppose rollup set options.acorn = Object.assign({}, options.acorn);
acorn.ecmaVersion = 7;
await rollup(options);// works unexpected

assert.deepStrictEqual won't be perfect in modification checking.

And I already add Object.freeze to all non-primitive value to achieve the purpose that checking the whole options object in the latest changes.

And, rollup do modification to the options currenctly infact (add plugin.name and delete options.cache), we can't check them before we adapt all of them. I added two comments in the test too, so as to check the in the future.

[lukastaegert]

I read it and agreed, sorry, there is a lot going on here.

[lukastaegert]

I think this is good to be merged