refactor(core/biblio-db): Move from indexedDB to idb #2228

Swapnilr1 · 2019-04-04T09:05:34Z

Fixes #1955

I started refactoring few days ago but ran into issues with tests and have not been able to resolve it.
When running the tests in biblio-db-spec.js, I get Error: Script error for "idb", needed by: core/biblio-db and the tests fail.
I even tested after removing almost all code and keeping just

import { openDB } from "idb";
export const name = "core/biblio-db";
export const biblioDB = {};

in biblio-db.js, and I still got the same error Error: Script error for "idb", needed by: core/biblio-db.
I even tried to use the IDB code from xref.js and I still got the same error. 😢

I noticed that the tests for xref include the database creation code:

await new Promise(resolve => require(["deps/idb"], resolve));
const { openDB } = window.idb;
const { IDBKeyVal } = await new Promise(resolve =>
  require(["core/utils"], resolve)
);
const idb = await openDB("xref", 1, {
  upgrade(db) {
    db.createObjectStore("xrefs");
  },
});

So I think I have to make changes to the test files to get idb to be "initialized".

Any guidance is much appreciated.

Swapnilr1 · 2019-04-04T09:49:58Z

@sidvishnoi Could you have a look and tell me how to resolve Script error in tests (details in description)?

saschanaz · 2019-04-04T11:40:33Z

Just tried fixing your error, it should now at least import the package properly.

Swapnilr1 · 2019-04-04T12:21:28Z

@saschanaz Thanks for your help, though the error still persist. But your changes pointed me to towards the problem: The test is trying to fetch js/idb.js and getting a 404 instead of js/deps/idb.js.
I tried importing idb in some other modules and found that tests fail with the Script error when the test spec includes a require statement like https://github.com/w3c/respec/blob/24bf82b40e2e2ccb9efccc04390264b25252db77/tests/spec/core/biblio-db-spec.js#L42 and work otherwise (though I think you already know this).

So I need to check how to set the correct path for tests. Working on it. Thanks a lot.
EDIT: I see in Developer Tools that idb.js is loaded twice, once from the wrong location giving a 404. But I don't really understand how to debug this.

Swapnilr1 · 2019-04-08T12:07:40Z

Finally fixed tests locally. Thank you @marcoscaceres and @saschanaz for help with importing idb correctly.
I have almost completed the refactor, please review it - I think I have wrongly removed the DOMException rejections.
I am looking into why the tests are failing, because they ran well locally in SpecRunner. 😕

saschanaz · 2019-04-08T12:24:30Z

I suspect you are using SpecRunner.html? Maybe that's why I was seeing different result...

Swapnilr1 · 2019-04-08T12:31:05Z

@saschanaz Yes I am using SpecRunner.html. Thanks for the fix.

src/core/biblio-db.js

saschanaz · 2019-04-09T00:10:03Z

src/core/biblio-db.js

-        return Array.from(aliasesAndRefs[type]).map(details =>
-          await this.add(type, details)
+        return Array.from(aliasesAndRefs[type]).map(
+          async details => await this.add(type, details)


This change is redundant because the return type is still Promise, and it's expected.

saschanaz · 2019-04-09T00:10:50Z

src/core/biblio-db.js

@@ -222,7 +221,7 @@ export const biblioDB = {
    const storeNames = [...ALLOWED_TYPES];
    const stores = await db.transaction(storeNames, "readwrite");
    const clearStorePromises = storeNames.map(name => {
-      return stores.objectStore(name).clear();
+      return await stores.objectStore(name).clear();


This is redundant too. It still returns a Promise anyway.

marcoscaceres

Couple of quick suggestions... not sure all apply.

src/core/biblio-db.js

marcoscaceres · 2019-04-09T01:08:45Z

src/core/biblio-db.js

-    });
+    const objectStore = db.transaction([type], "readonly").objectStore(type);
+    const range = IDBKeyRange.only(id);
+    return !!(await objectStore.openCursor(range));


I know this was in the original, but it's still a bit ugly... maybe we can do?

Suggested change

return !!(await objectStore.openCursor(range));

const result = await objectStore.openCursor(range);

return result ? true : false;

Same with other one below...

? true : false? It doesn't look JavaScript-y for me... Maybe !!result?

(Because result ? part already coerces result into a boolean.)

!!result wfm

marcoscaceres · 2019-04-09T01:10:01Z

src/core/biblio-db.js

+      .transaction(["alias"], "readonly")
+      .objectStore("alias");
+    const range = IDBKeyRange.only(id);
+    return !!(await objectStore.openCursor(range));


As above...

marcoscaceres · 2019-04-09T01:12:59Z

src/core/biblio-db.js

-    });
+    const store = db.transaction([type], "readwrite").objectStore(type);
+    // update or add, depending of already having it in db
+    const request = isInDB ? store.put(details) : store.add(details);


Maybe we should do:
Maybe just:

Suggested change

const request = isInDB ? store.put(details) : store.add(details);

return isInDB ? await store.put(details) : await store.add(details);

Swapnilr1 · 2019-04-09T22:23:41Z

I have implemented the changes, but I am not able to run tests either by SpecRunner.html or by karma. SpecRunner gives SyntaxError: import declarations may only appear at top level of a module while the karma tests simply time out (locally as well as on Travis)

saschanaz · 2019-04-10T04:02:02Z

Investigation incoming...

saschanaz · 2019-04-10T06:23:55Z

The problem is that currently the ES module system does not support resolving by package name. After #2241, karma wants to load idb as a dependency of core/biblio-db.js, which fails because the module system does not know what idb is.

We can replace idb with ../../node_modules/idb/build/esm/index.js as a workaround. It's not pretty, but anyway...

marcoscaceres · 2019-04-10T06:28:54Z

Yeah, this is why copydeps.js exists :( and what’s nice about webpack is that it handles this. I’m still not sure how we overcome copydeps.js in a modules only world. We probably can’t as those files have to live somewhere.

Swapnilr1 · 2019-04-10T13:33:15Z

We can replace idb with ../../node_modules/idb/build/esm/index.js as a workaround.

@saschanaz This breaks everything else except the karma tests. This is because index.js is served directly to the browser and browsers do not support import. I think reverting to import { openDB } from "idb"; and adding a shim would solve the issue, but I am not sure where to add the shim.

saschanaz · 2019-04-10T13:41:51Z

Browsers do support imports, but yes, it will break requirejs compatibility... 🤔

Swapnilr1 · 2019-04-10T13:52:03Z

Browsers do support imports, ...

Oh, I thought browsers not supporting imports was the reason SpecRunner.html was failing, but I believe browsers require a type="module" attribute which RequireJS does not provide. I need help with configuring shims.

saschanaz · 2019-04-11T02:03:49Z

Seems we are kind of locked. Maybe we should do #1975 first?

marcoscaceres · 2019-04-24T15:32:55Z

Now that SpecRunner is gone, @Swapnilr1 what’s blocking this?

Swapnilr1 · 2019-04-24T17:00:04Z

@marcoscaceres SpecRunner wasn't really blocking this (it doesn't run on Travis anyway) - SpecRunner would not work after #2241 so I had just made a comment regarding that.
The blocker is karma tests. Using import { openDB } from "idb"; (which loads the IIFE build) leads to TypeError: Failed to resolve module specifier "idb". when running karma tests. (Most likely because we require a shim - though I am not sure if it can be done).

Switching to import { openDB } from "../../node_modules/idb/build/esm/index.js"; as suggested by @saschanaz fixes the issue with karma tests but will break everything else. This is because index.js has import/export declarations, and unlike other modules won't be compiled by Babel before being served to the browser. The browser then complains of import declarations in a non-module because RequireJS does not serve it as an ES Module.

[Extremely sorry for any technical inaccuracy; I am not very well versed with ES6 modules].

marcoscaceres · 2019-04-24T17:02:41Z

Hhmm... should we complain upstream? This doesn’t sound good and no other module exhibits this kind of problem?

Swapnilr1 · 2019-04-24T17:14:07Z

@marcoscaceres Yes, perhaps we can ask for RequireJS compatibility without needing shims - though I am not completely certain if this will fix the problem.

saschanaz · 2019-04-25T00:18:28Z

This is because karma now consumes ES modules and this PR tries consuming a non-ES module (or ES module without an actual path) in an ES module. Webpack and requirejs somehow can deal with it but karma need import-maps support from browsers (to map "idb" to the actual path).

marcoscaceres · 2019-04-25T02:15:16Z

this PR tries consuming a non-ES module (or ES module without an actual path) in an ES module

Ok, but that seems like a bug upstream, no? The maintainer has shipped something non-conforming. We could ask them to ship an actual ES module, instead of whatever this is.

saschanaz · 2019-04-25T02:25:46Z

No, idb itself is good, what happens here is we cannot just import * from "idb" because the native ES module system does not support non-path import. It's okay for marked and hyperhtml because they are not directly imported from karma (as generally tests just uses the compiled full ReSpec), but here we want to directly import a module and write a unit test for it.

marcoscaceres · 2019-04-25T02:52:25Z

No, idb itself is good, what happens here is we cannot just import * from "idb" because the native ES module system does not support non-path import.

That makes sense :)

It's okay for marked and hyperhtml because they are not directly imported from karma (as generally tests just uses the compiled full ReSpec), but here we want to directly import a module and write a unit test for it.

This doesn't make sense to me (and probably where I'm getting confused). We should be testing the idb that is compiled into ReSpec. Additionally, shouldn't we just use use copydeps.js to copy idb over to deps?

copydeps.js can be removed once we fully transition to some packager.

saschanaz · 2019-04-25T02:58:07Z

We should be testing the idb that is compiled into ReSpec.

We have tests that directly imports biblio-db as a module, which now depends on idb in this PR.

shouldn't we just use use copydeps.js to copy idb over to deps?

Currently copydeps is only used for requirejs mapping, otherwise TypeScript type system breaks (as they only knows package name to type).

It doesn't help here anyway, if we copy cjs then ES module breaks, or we copy esm then requirejs breaks, so 🤷‍♀️

marcoscaceres · 2019-04-25T02:59:58Z

Yay! module systems 💃. Ok, we hold I guess until we kill RequiresJS.

saschanaz · 2019-04-25T03:01:55Z

until we kill RequiresJS.

Or until we get import maps on Firefox. I hope Mozillians get some push for this new feature.

marcoscaceres · 2019-04-25T03:04:00Z

I believe this is appropriate?:

😂

sidvishnoi · 2019-07-14T14:30:15Z

What's blocking this one? Why can't we use idb the way we used in xref.js and xref-spec.js?

saschanaz · 2019-07-15T08:45:13Z

What's blocking this one? Why can't we use idb the way we used in xref.js and xref-spec.js?

Because Karma is using ES module, some tests there want to import core/biblio-db, and then the ES module system doesn't know where idb maps to. core/xref is not directly imported within karma, however.

Using the actual path for ES module version of idb will solve the problem and then will break RequireJS because it doesn't support ES module.

saschanaz · 2019-08-03T00:54:58Z

Okay, the tests now pass. Now time to review again! @marcoscaceres @sidvishnoi

marcoscaceres

Just a small nit.

marcoscaceres · 2019-08-03T01:10:19Z

src/core/biblio-db.js

@@ -81,7 +45,8 @@ export const biblioDB = {
    if (await this.isAlias(id)) {
      id = await this.resolveAlias(id);
    }
-    return this.get("reference", id);
+    const result = this.get("reference", id);


We probably want to await this.get, then return the result.

marcoscaceres · 2019-08-03T01:46:11Z

@saschanaz, it shows you as having requested changes? I guess that’s from an old review?

saschanaz

👍 Sorry for the loooong delay and thank you for this PR @Swapnilr1!

Starting to refactor

215430a

Swapnilr1 mentioned this pull request Apr 4, 2019

Refactor biblio-db with idb dependency #1955

Closed

fix requirejs error

428ab9f

Swapnilr1 added 2 commits April 8, 2019 14:27

Merge branch 'develop' of github.com:w3c/respec into bibliodbIDB

0ed5466

Fixed issue with tests, refactoring completed

825c027

Swapnilr1 marked this pull request as ready for review April 8, 2019 11:36

Swapnilr1 and others added 2 commits April 8, 2019 17:07

Merge branch 'develop' into bibliodbIDB

9a63ba6

lint fix

d6864e1

fix idb import on xref

ccf44d1

saschanaz reviewed Apr 8, 2019

View reviewed changes

src/core/biblio-db.js Outdated Show resolved Hide resolved

saschanaz requested changes Apr 8, 2019

View reviewed changes

src/core/biblio-db.js Outdated Show resolved Hide resolved

src/core/biblio-db.js Outdated Show resolved Hide resolved

src/core/biblio-db.js Outdated Show resolved Hide resolved

Swapnilr1 added 2 commits April 8, 2019 19:10

Apply suggested corrections

1c7ba6e

async and lint fixes

c0bdaa2

saschanaz reviewed Apr 8, 2019

View reviewed changes

src/core/biblio-db.js Outdated Show resolved Hide resolved

Removed redundant promises

3651243

saschanaz requested changes Apr 9, 2019

View reviewed changes

marcoscaceres requested changes Apr 9, 2019

View reviewed changes

Swapnilr1 added 3 commits April 10, 2019 02:16

Merge branch 'develop' of github.com:w3c/respec into bibliodbIDB

5360af1

Applied suggested changes

00cd41e

lint fix

af36620

Merge branch 'develop' of github.com:w3c/respec into bibliodbIDB

3b303f3

Merge branch 'develop' into bibliodbIDB

ee70186

Merge branch 'develop' into bibliodbIDB

e2734e4

saschanaz mentioned this pull request Aug 2, 2019

refactor(core/xref): split its cache feature #2463

Merged

saschanaz added 2 commits August 3, 2019 09:18

Merge branch 'develop' into bibliodbIDB

576d1e5

use new importIdb()

56899a0

marcoscaceres approved these changes Aug 3, 2019

View reviewed changes

return await

d8fdae7

saschanaz approved these changes Aug 3, 2019

View reviewed changes

saschanaz merged commit f05ad79 into speced:develop Aug 3, 2019

	return !!(await objectStore.openCursor(range));
	const result = await objectStore.openCursor(range);
	return result ? true : false;

	const request = isInDB ? store.put(details) : store.add(details);
	return isInDB ? await store.put(details) : await store.add(details);

refactor(core/biblio-db): Move from indexedDB to idb #2228

refactor(core/biblio-db): Move from indexedDB to idb #2228

Conversation

Swapnilr1 commented Apr 4, 2019 • edited Loading

Swapnilr1 commented Apr 4, 2019 • edited Loading

saschanaz commented Apr 4, 2019

Swapnilr1 commented Apr 4, 2019 • edited Loading

Swapnilr1 commented Apr 8, 2019 • edited Loading

saschanaz commented Apr 8, 2019

Swapnilr1 commented Apr 8, 2019

saschanaz Apr 9, 2019

Choose a reason for hiding this comment

saschanaz Apr 9, 2019

Choose a reason for hiding this comment

marcoscaceres left a comment

Choose a reason for hiding this comment

marcoscaceres Apr 9, 2019

Choose a reason for hiding this comment

saschanaz Apr 9, 2019 • edited Loading

Choose a reason for hiding this comment

marcoscaceres Apr 9, 2019

Choose a reason for hiding this comment

marcoscaceres Apr 9, 2019

Choose a reason for hiding this comment

marcoscaceres Apr 9, 2019

Choose a reason for hiding this comment

Swapnilr1 commented Apr 9, 2019 • edited Loading

saschanaz commented Apr 10, 2019

saschanaz commented Apr 10, 2019 • edited Loading

marcoscaceres commented Apr 10, 2019

Swapnilr1 commented Apr 10, 2019 • edited Loading

saschanaz commented Apr 10, 2019

Swapnilr1 commented Apr 10, 2019

saschanaz commented Apr 11, 2019

marcoscaceres commented Apr 24, 2019

Swapnilr1 commented Apr 24, 2019

marcoscaceres commented Apr 24, 2019

Swapnilr1 commented Apr 24, 2019

saschanaz commented Apr 25, 2019 • edited Loading

marcoscaceres commented Apr 25, 2019

saschanaz commented Apr 25, 2019

marcoscaceres commented Apr 25, 2019

saschanaz commented Apr 25, 2019 • edited Loading

marcoscaceres commented Apr 25, 2019

saschanaz commented Apr 25, 2019

marcoscaceres commented Apr 25, 2019

sidvishnoi commented Jul 14, 2019

saschanaz commented Jul 15, 2019

saschanaz commented Aug 3, 2019

marcoscaceres left a comment

Choose a reason for hiding this comment

marcoscaceres Aug 3, 2019

Choose a reason for hiding this comment

marcoscaceres commented Aug 3, 2019

saschanaz left a comment

Choose a reason for hiding this comment

Swapnilr1 commented Apr 4, 2019 •

edited

Loading

Swapnilr1 commented Apr 4, 2019 •

edited

Loading

Swapnilr1 commented Apr 4, 2019 •

edited

Loading

Swapnilr1 commented Apr 8, 2019 •

edited

Loading

saschanaz Apr 9, 2019 •

edited

Loading

Swapnilr1 commented Apr 9, 2019 •

edited

Loading

saschanaz commented Apr 10, 2019 •

edited

Loading

Swapnilr1 commented Apr 10, 2019 •

edited

Loading

saschanaz commented Apr 25, 2019 •

edited

Loading

saschanaz commented Apr 25, 2019 •

edited

Loading