Add an exception for ESLint no-self-compare in test_complex_keyPaths.js as the comparisons are intentional.

RESOLVED FIXED in Firefox 58

Status

()

enhancement
P2
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: standard8, Assigned: standard8)

Tracking

Trunk
mozilla58
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox58 fixed)

Details

Attachments

(1 attachment)

Assignee

Description

2 years ago
In bug 1371293, I'm working on upgrading to ESLint 4. One of the current issues is in test_complex_keyPaths.js:

dom/indexedDB/test/unit/test_complex_keyPaths.js
   88:12  error  Comparing to itself is potentially pointless.  no-self-compare (eslint)
  177:12  error  Comparing to itself is potentially pointless.  no-self-compare (eslint)

In both cases, the code is similar.

First snippet:

    let objectStore = db.createObjectStore(indexName, { keyPath: info.keyPath });
    ok(!("exception" in info), "shouldn't throw" + test);
    is(JSON.stringify(objectStore.keyPath), JSON.stringify(info.keyPath),
       "correct keyPath property" + test);
    ok(objectStore.keyPath === objectStore.keyPath,
       "object identity should be preserved");


    let index = store.createIndex(indexName, info.keyPath);
    ok(!("exception" in info), "shouldn't throw" + test);
    is(JSON.stringify(index.keyPath), JSON.stringify(info.keyPath),
       "index has correct keyPath property" + test);
    ok(index.keyPath === index.keyPath,
       "object identity should be preserved");

with the last line failing.

Should those right-hand values for the ok(... === ...) be info.keyPath? If so, I don't quite understand why the need for the stringify before it.

Alternately, should we delete the ok checks?
Flags: needinfo?(jvarga)
(In reply to Mark Banner (:standard8) from comment #0)
> Should those right-hand values for the ok(... === ...) be info.keyPath? If
> so, I don't quite understand why the need for the stringify before it.
> 
> Alternately, should we delete the ok checks?

This was added in bug 767186. According to the patch, before bug 767186 was fixed, when calling {IDBObjectStore|IDBIndex}.keyPath a new JS value will always be created internally from a KeyPath instance and returned.
Because this property are readonly and will never be changed internally, this self-comparison of === is necessary to ensure that the same JS object will be returned every time when {IDBObjectStore|IDBIndex}.keyPath is accessed.

In addition, the keyPath could be either an string or an array of string according to
http://w3c.github.io/IndexedDB/#key-path-construct
Hence, stringifying them simplifies the comparison.
Flags: needinfo?(jvarga)
Priority: -- → P2
This is an intended comparison shall be an exception to the rule of "no-self-compare" in ESLint per comment 1.
I should set this as invalid in comment 1 as well without waiting for the prioritization.
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → INVALID
Assignee

Comment 3

2 years ago
Let's use this bug for adding the exception, then we've got all the information together.

I'll have a patch up in a few.
Assignee: nobody → standard8
Status: RESOLVED → REOPENED
Resolution: INVALID → ---

Comment 5

2 years ago
mozreview-review
Comment on attachment 8918202 [details]
Bug 1407968 - Add an exception for ESLint no-self-compare in test_complex_keyPaths.js as the comparisons are intentional.

https://reviewboard.mozilla.org/r/189084/#review194382
Attachment #8918202 - Flags: review?(btseng) → review+
Summary: Potentially pointless comparisions in test_complex_keyPaths.js (ESLint 4) → Add an exception for ESLint no-self-compare in test_complex_keyPaths.js as the comparisons are intentional.

Comment 6

2 years ago
Pushed by mbanner@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/885aeb78d652
Add an exception for ESLint no-self-compare in test_complex_keyPaths.js as the comparisons are intentional. r=bevis
https://hg.mozilla.org/mozilla-central/rev/885aeb78d652
Status: REOPENED → RESOLVED
Last Resolved: 2 years ago2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
You need to log in before you can comment on or make changes to this bug.