Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

avoid to change original options (.acorn) object #3051

Merged
merged 29 commits into from Aug 14, 2019
Merged

avoid to change original options (.acorn) object #3051

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
d1700c0
Update Graph.ts
LongTengDao Aug 12, 2019
c46f9fd
Merge branch 'master' into master
shellscape Aug 13, 2019
1032015
Update misc.js
LongTengDao Aug 13, 2019
767427b
Update misc.js
LongTengDao Aug 13, 2019
be2ba99
Merge branch 'master' into master
LongTengDao Aug 13, 2019
bd062ee
Update misc.js
LongTengDao Aug 13, 2019
2a8aae0
Update misc.js
LongTengDao Aug 13, 2019
d1e3acc
Update misc.js
LongTengDao Aug 13, 2019
905905a
Update misc.js
LongTengDao Aug 13, 2019
b8e07ed
Update misc.js
LongTengDao Aug 13, 2019
0ebde99
Update misc.js
LongTengDao Aug 13, 2019
2add7e0
Update index.ts
LongTengDao Aug 13, 2019
ea5e298
Update utils.js
LongTengDao Aug 13, 2019
838932e
Update misc.js
LongTengDao Aug 13, 2019
90d456c
Update misc.js
LongTengDao Aug 13, 2019
c72c386
Update misc.js
LongTengDao Aug 13, 2019
1241296
Update misc.js
LongTengDao Aug 13, 2019
a4b7d77
Update misc.js
LongTengDao Aug 13, 2019
f5a33eb
Update misc.js
LongTengDao Aug 13, 2019
c2f5e16
Update index.ts
LongTengDao Aug 13, 2019
e13e23c
Update utils.js
LongTengDao Aug 13, 2019
9c3e576
Update index.ts
LongTengDao Aug 13, 2019
1b2669e
Update index.ts
LongTengDao Aug 13, 2019
0c559b2
Update utils.js
LongTengDao Aug 13, 2019
4c34fd2
Update misc.js
LongTengDao Aug 13, 2019
a539a77
Merge branch 'master' into master
lukastaegert Aug 13, 2019
b7f3978
Update misc.js
LongTengDao Aug 14, 2019
a8d4fd2
Merge branch 'master' into master
lukastaegert Aug 14, 2019
3e13819
Remove freezes where it is unlikely Rollup will fix them
lukastaegert Aug 14, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/Graph.ts
View file
Open in desktop
Expand Up @@ -166,7 +166,7 @@ export default class Graph {
this.getModuleContext = () => this.context;
}

this.acornOptions = options.acorn || {};
this.acornOptions = options.acorn ? Object.assign({}, options.acorn) : {};
const acornPluginsToInject = [];

acornPluginsToInject.push(injectImportMeta);
Expand Down
21 changes: 21 additions & 0 deletions test/misc/misc.js
View file
Open in desktop
Expand Up @@ -3,6 +3,27 @@ const rollup = require('../../dist/rollup');
const { loader } = require('../utils.js');

describe('misc', () => {
it('throw modification of options or its property', () => {
const { freeze } = Object;
return rollup.rollup(
freeze({
input: 'input',
external: freeze([]),
plugins: freeze([
{
name: 'loader',
resolveId: freeze(() => 'input'),
load: freeze(() => `export default 0;`)
}
]),
acornInjectPlugins: freeze([]),
acorn: freeze({}),
experimentalTopLevelAwait: true,
treeshake: freeze({})
})
);
});

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

it('warns if node builtins are unresolved in a non-CJS, non-ES bundle (#1051)', () => {
const warnings = [];

Expand Down
1 change: 1 addition & 0 deletions test/utils.js
View file
Open in desktop
Expand Up @@ -117,6 +117,7 @@ function removeOldTest(dir) {
}

function loader(modules) {
modules = Object.assign(Object.create(null), modules);
return {
resolveId(id) {
return id in modules ? id : null;
Expand Down