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

no-new-object to allow argument #11810

Closed
brettz9 opened this issue Jun 6, 2019 · 7 comments
Closed

no-new-object to allow argument #11810

brettz9 opened this issue Jun 6, 2019 · 7 comments
Labels
archived due to age This issue has been archived; please open a new issue for any further discussion auto closed The bot closed this issue enhancement This change enhances an existing feature of ESLint evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion rule Relates to ESLint's core rules

Comments

@brettz9
Copy link
Contributor

brettz9 commented Jun 6, 2019

What rule do you want to change?

no-new-object

Does this change cause the rule to produce more or fewer warnings?

Fewer warnings.

How will the change be implemented? (New option, new default behavior, etc.)?

New default behavior.

Please provide some example code that this change will affect:

new Object(BigInt('1n'))

What does the rule currently do for this code?

Reports an error.

What will the rule do after it's changed?

Not report anything.

Are you willing to submit a pull request to implement this change?

Yes.

As per the above example, there are uses for wrapping some items within a new Object() (or within the equivalent, Object(), but some prefer the style of requiring new in such cases, as per unicorn/new-for-builtins).

If it is thought that people will like to make this optional, I can modify to something like allowWithArguments.

@brettz9 brettz9 added enhancement This change enhances an existing feature of ESLint rule Relates to ESLint's core rules triage An ESLint team member will look at this issue soon labels Jun 6, 2019
brettz9 added a commit to brettz9/eslint that referenced this issue Jun 6, 2019
brettz9 added a commit to brettz9/eslint that referenced this issue Jun 6, 2019
brettz9 added a commit to brettz9/eslint that referenced this issue Jun 6, 2019
@platinumazure
Copy link
Member

This should definitely be optional. 😄

@platinumazure platinumazure added evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion and removed triage An ESLint team member will look at this issue soon labels Jun 6, 2019
@mysticatea
Copy link
Member

Out of curiosity, why do you want to use the wrapper objects?

brettz9 added a commit to brettz9/eslint that referenced this issue Jun 7, 2019
brettz9 added a commit to brettz9/eslint that referenced this issue Jun 7, 2019
brettz9 added a commit to brettz9/eslint that referenced this issue Jun 7, 2019
brettz9 added a commit to brettz9/eslint that referenced this issue Jun 7, 2019
@brettz9
Copy link
Contributor Author

brettz9 commented Jun 7, 2019

I've changed it to be optional and off-by-default (and switching the commit message from "Breaking" to "New").

My own need is merely for typeson-registry's ability to serialize/deserialize BigInt objects in case anyone uses them within the structured cloning algorithm. And we're using typeson-registry with IndexedDBShim to allow it properly store such objects. And I was motivated to do this primarily because the W3C tests include this syntax :)

But I think some may wish to use this for polyfilling or extending the prototype. Interestingly, Object() converts to a primitive's more closely corresponding object, e.g., BigInt, Number, Boolean, String, so it is not just for polyfilling/enhancing Object. In the case of BigInt, therefore, one could add special arithmetic methods on BigInt.prototype. For a dumb example:

BigInt.prototype.plusOne = function () {
    return this + 1n;
};

Object(1n).plusOne();

@mysticatea
Copy link
Member

Because JS runtime does boxing automatically, so you don't need to wrap it by Object(). I.e., 1n.plusOne() works as expected.

@brettz9
Copy link
Contributor Author

brettz9 commented Jun 7, 2019

Btw, the reason I originally suggested the behavior as the default is because the only rationale given in the docs for avoiding new Object is that it is equivalent to the object literal, but in this case it isn't.

However, I understand people don't always want to allow such more obscure uses.

brettz9 added a commit to brettz9/eslint that referenced this issue Jun 7, 2019
brettz9 added a commit to brettz9/eslint that referenced this issue Jun 7, 2019
@brettz9
Copy link
Contributor Author

brettz9 commented Jun 7, 2019

Ah, ok, I see, thanks, @mysticatea . I've amended the docs to avoid mentioning that. But it is still possible to do such conversions. I've also seen Object() used to polyfill certain built-in methods, e.g., https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/find#Polyfill (as part of ToObject), though this seems pretty obscure here too--I am not sure what case this covers, however (it already seems to be an Object when it gets to that point, so maybe it is not needed in the polyfill).

brettz9 added a commit to brettz9/eslint that referenced this issue Jun 30, 2019
@eslint-deprecated eslint-deprecated bot added the auto closed The bot closed this issue label Jul 8, 2019
@eslint-deprecated
Copy link

Unfortunately, it looks like there wasn't enough interest from the team
or community to implement this change. While we wish we'd be able to
accommodate everyone's requests, we do need to prioritize. We've found
that issues failing to reach accepted status after 21 days tend to
never be accepted, and as such, we close those issues.
This doesn't mean the idea isn't interesting or useful, just that it's
not something the team can commit to.

Thanks for contributing to ESLint and we appreciate your understanding.

@eslint-deprecated eslint-deprecated bot locked and limited conversation to collaborators Jan 5, 2020
@eslint-deprecated eslint-deprecated bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Jan 5, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
archived due to age This issue has been archived; please open a new issue for any further discussion auto closed The bot closed this issue enhancement This change enhances an existing feature of ESLint evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion rule Relates to ESLint's core rules
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants