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.

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

Allow Object.create(null) in no-mutating-assign #38

Merged
merged 1 commit into from Feb 14, 2018

Conversation

macklinu
Copy link
Contributor

Sometimes I see Object.create(null) as a first argument of Object.assign(), like in eslint-plugin-jest's codebase. Thoughts on including it as an acceptable first argument in the no-mutating-assign rule?

Copy link
Owner

@jfmengels jfmengels left a comment

Choose a reason for hiding this comment

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

I don't see why not 👍
That said, I'm sure there are plenty of functions that create a new value (_.cloneDeep(), sometimes a.map(), etc.) that we may want to add as exceptions at some point, which may be hard to maintain, but I don't mind adding this.

I think any call to Object.create(xyz) should be allowed, regardless of the argument, as I don't think it mutates anything and always creates a new object

@@ -23,6 +23,8 @@ ruleTester.run('no-mutating-assign', rule, {
'Object.foo(a, b);',
'Object.assign({});',
'Object.assign({}, b);',
'Object.assign(Object.create(null));',
Copy link
Owner

@jfmengels jfmengels Feb 14, 2018

Choose a reason for hiding this comment

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

Could you add the following tests?
Valid:

  • 'Object.assign(Object.create(a))' // Any call of Object.create should probably be allowed

Invalid:

  • 'Object.assign(Object.create)' // We don't want to mutate the function

Copy link
Collaborator

Choose a reason for hiding this comment

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

assigning to Object.create(v) can have side effects and mutate things: https://runkit.com/graingert/mutate

Copy link
Owner

Choose a reason for hiding this comment

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

Ah, you're right. Scrap the valid test I proposed then

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Pushed up the invalid test case, @jfmengels 👍

@graingert graingert merged commit 205861e into jfmengels:master Feb 14, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants