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.
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
avoid to change original options (.acorn) object #3051
Merged
Merged
Changes from 25 commits
Commits
Show all changes
29 commits
Select commit
Hold shift + click to select a range
d1700c0
Update Graph.ts
LongTengDao c46f9fd
Merge branch 'master' into master
shellscape 1032015
Update misc.js
LongTengDao 767427b
Update misc.js
LongTengDao be2ba99
Merge branch 'master' into master
LongTengDao bd062ee
Update misc.js
LongTengDao 2a8aae0
Update misc.js
LongTengDao d1e3acc
Update misc.js
LongTengDao 905905a
Update misc.js
LongTengDao b8e07ed
Update misc.js
LongTengDao 0ebde99
Update misc.js
LongTengDao 2add7e0
Update index.ts
LongTengDao ea5e298
Update utils.js
LongTengDao 838932e
Update misc.js
LongTengDao 90d456c
Update misc.js
LongTengDao c72c386
Update misc.js
LongTengDao 1241296
Update misc.js
LongTengDao a4b7d77
Update misc.js
LongTengDao f5a33eb
Update misc.js
LongTengDao c2f5e16
Update index.ts
LongTengDao e13e23c
Update utils.js
LongTengDao 9c3e576
Update index.ts
LongTengDao 1b2669e
Update index.ts
LongTengDao 0c559b2
Update utils.js
LongTengDao 4c34fd2
Update misc.js
LongTengDao a539a77
Merge branch 'master' into master
lukastaegert b7f3978
Update misc.js
LongTengDao a8d4fd2
Merge branch 'master' into master
lukastaegert 3e13819
Remove freezes where it is unlikely Rollup will fix them
lukastaegert File filter
Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
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:
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 noname
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.There was a problem hiding this comment.
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 ;)
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.
There was a problem hiding this comment.
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.