Remove correctness/noDelete from the default/recommended rules
#3614
Replies: 8 comments 8 replies
-
|
Thanks @lgarron for starting this discussion and for your extensive explanation. You raise many valid points. What I take from your explanation is that there are three issues: a) Unclear fix: The rule doesn't make it clear what the consequences of the suggested fix are and fails to point out other alternatives (using spread My experience has been that I only rarely needed the
The default behaviour of JSON is to ignore undefined What are the tools that you use that deviate from this behavior? |
Beta Was this translation helpful? Give feedback.
-
Yeah, I think this would make a lot more sense. You make some good points, and I feel like I've learned some things in this very thread! But I'm still rather concerned about correctness within a running JS environment, where (FWIW, in the main codebase I'm using with |
Beta Was this translation helpful? Give feedback.
-
|
Sounds good. Let me create an issue on Monday to move the rule to a new performance group because the rule isn't about correctness (it doesn't fix any potential bugs). Part of that will be to better explain the motivation. I'll create another issue to suggest destructuring as an alternative fix. |
Beta Was this translation helpful? Give feedback.
This comment was marked as spam.
This comment was marked as spam.
-
|
@MichaReiser I see the The only instances I have in my codebase are ones where I need |
Beta Was this translation helpful? Give feedback.
-
|
This is probably not something we've made clear enough but |
Beta Was this translation helpful? Give feedback.
-
|
While this might be important to consider in a hot-loop, it can be totally fine in other cases (e.g: removing an item from a cart in a click event handler). Maybe this lint level could be lowered to a warning ? |
Beta Was this translation helpful? Give feedback.
-
|
noDelete was added a few years back in PR #350. The delete keyword is intended for use with object properties. Its behavior on array entries is only understandable - not exactly intended. Here's a very MRE: const cache = Object.create(null)
function cachePut (key, value) { cache[key] = value }
function cacheGet (key) { return cache[key] }
function cacheDel (key) { delete cache[key] }And something to paste into a .mjs: class Cache {
#lifespan = undefined
#entries = Object.create(null)
constructor (lifespan) {
this.#lifespan = lifespan
}
get (key) {
return this.#entries[key]?.value
}
put (key, value) {
this.#entries[key] = Object.assign(Object.create(null), {
expires: Date.now() + this.#lifespan,
value
})
}
del (key) {
delete this.#entries[key]
}
clean (hard) {
if (hard) {
this.#entries = Object.create(null)
return
}
const time = Date.now()
for (const key in this.#entries)
if (this.#entries[key].expires < time)
delete this.#entries[key]
}
}
const blah = new Cache(1000)
blah.put("cat", "dog")
console.log(blah.get("cat"))
blah.clean()
console.log(blah.get("cat"))
await new Promise((done) => setTimeout(done, 2000))
blah.clean()
console.log(blah.get("cat"))Especially in long life processes, node, deno, or bun, etc, this kind of object-backed cache is common for holding ephemeral data, such as (source => token) pairs for logins. The reason the noDelete rule is a problem in this circumstance is that assigning undefined to a key does not delete the key. Over the lifespan of a long-lived app, such an object may easily accumulate a large hidden debt of keys -- and stringification in common JS engines actually hides this problem by being clever. As a very simple example with noise removed: The application language use case conflicts with the article mentioned in #3614 (comment). The reason for that is that the two targets are very different - The article talks about the delete keyword negatively impacting webgl rendering code with crafted examples. I can see that being a thing. I don't know where the tradeoff begins. One might circumvent the warning in either case by using Map or recomposition and replacement. I believe the rule is too broad and not valid, except in reference to the original part of this post, for style reasons - no delete on array indices. |
Beta Was this translation helpful? Give feedback.
-
|
Taking feedbacks into account I could suggest relaxing the rule: we could allow The following pattern would pass: delete record[computed]While the following patterns would still be rejected: delete record["literal"]
delete record[0]
delete record.a |
Beta Was this translation helpful? Give feedback.
-
|
Unfortunately, rejecting |
Beta Was this translation helpful? Give feedback.
-
|
@lgarron Have you some real code example of this kind of usage? |
Beta Was this translation helpful? Give feedback.
-
|
https://github.com/cubing/cubing.js/ currently uses this pattern in 8 places. |
Beta Was this translation helpful? Give feedback.
-
|
You can avoid all In - const { ...def } = kpuzzle.definition;
- delete def.experimentalIsStateSolved;
+ const { experimentalIsStateSolved, ...def } = kpuzzle.definition;This avoids mutating the object shape, and is shorter. In the same vein in - const config = getConfigFromURL("", this.a.href);
+ const { puzzle, ...config} = getConfigFromURL("", this.a.href);
- if (config.puzzle && !(config.puzzle in puzzles)) {
+ if (puzzle && !(puzzle in puzzles)) {This is also shorter. This has also the advantage to avoid a mutation that is a common assumption for config objects... |
Beta Was this translation helpful? Give feedback.
-
This is not true in general, if you are modifying an object that you do not have the only reference to.
I'll grant you that it works here, but 1) sometimes mutation is the point, and 2) while I'm a big fan of destructuring, I'm not sure I the result is easier to understand, especially for people who are not JS nerds. Also, how would you use this to delete an entry from |
Beta Was this translation helpful? Give feedback.
-
|
Another case where you can't avoid Edit: I'm wrong, there is one other way: you can use |
Beta Was this translation helpful? Give feedback.
-
Environment information
What happened?
Rome errors with
This is an unexpected use of the delete operator.for the following code:Playground link
Expected result
I would strongly prefer it if Rome did not error or warn on the
deleteoperator by default.As in the example above, it is a common pattern to use an object (Typescript
Record) to hold several potential fields, and for the absence of a field to be treated differently from that value being set toundefined. In the example above,"displayName" in settingsis a fairly common way to test whether a field is present.If the
deleteline is replaced withsettings["displayName"] = undefined;, then the wrongifbranch is taken, which is very undesirable. Even if it's a suggestion rather than an automatic fix, I would be worried that this could introduce bugs. Even a fairly thorough test suite might miss a code replacement bug like this, given that this particularly concerns edge cases forifstatements.In theory,
"displayName" in settingscould be replaced with a different check. One "obvious" initial replacement is to doif(settings.displayName), but this would be incorrect in cases where an empty string or another false-ish value (e.g.0for a number field) should not be treated the same as a missing field. A "proper" fix would betypeof settings.displayName !== "undefined", but 1) this is significantly less obvious, 2) Rome does not recommend this at the moment, and 3) this can have unintended effects like affecting bundle size if the code contains a lot of such checks.In addition, adding
undefinedvalues to fields can lead to other surprising behaviour.undefinedthe same as a missing field by default. In particular, other parsers and schemas might not accept such values, and the suggested fix could cause breakage.undefinedcan unexpectedly change the serialization or key set of an object, which may cause equality checks and optimizations to fail, and audit logs to reflect "ghost" changes.In essence, I am advocating that it is common and reasonable for codebases to stick with the convention that fields should be
deleted rather thanundefined, and that the default/recommended Rome rules should not cause friction with this convention. The simplest way to achieve this would be to makecorrectness/noDeleteopt-in.At the moment, I expect to disable this rule for every project, but 1) removing the rule by default would help for files outside projects with an explicit
rome.jsonfile, and 2) I would really love to be able to use and recommend Rome defaults without a "gotcha" like this.Code of Conduct
Beta Was this translation helpful? Give feedback.
All reactions