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

Status: RESOLVED FIXED

(Core :: Storage: IndexedDB, enhancement, P2)

Description

[Mark Banner (:standard8)]

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?

Comment 1

[Bevis Tseng[:bevis][:btseng](Exited)]

(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.

Comment 2

[Bevis Tseng[:bevis][:btseng](Exited)]

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.

Comment 3

[Mark Banner (:standard8)]

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.

Comment 4

[Mark Banner (:standard8)]

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

Review commit: https://reviewboard.mozilla.org/r/189084/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/189084/

Comment 5

[Bevis Tseng[:bevis][:btseng](Exited)]

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

Comment 6

[Pulsebot]

Pushed by [e-mail]:
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

Comment 7

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/885aeb78d652