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

4.0.0 #93

Merged
merged 79 commits into from
Mar 15, 2019
Merged

4.0.0 #93

merged 79 commits into from
Mar 15, 2019

Conversation

jakearchibald
Copy link
Owner

No description provided.

@jakearchibald
Copy link
Owner Author

@jeffposnick @philipwalton @inexorabletash as users of this library / IDB experts, I'd really appreciate a review of the readme for the new version.

Probably easier to look at https://github.com/jakearchibald/idb/blob/4.0.0/README.md than the diff.

@quasicomputational
Copy link

Road-testing the new version is running into a blocking problem for me: fake-indexeddb accesses cursor.source, but it's turning out to be undefined when the cursor's from async iteration. Here's a little reproducer:

import { openDB } from "idb/with-async-ittr.mjs";
import indexedDB from "fake-indexeddb";
import IDBKeyRange from 'fake-indexeddb/lib/FDBKeyRange';
import IDBIndex from 'fake-indexeddb/lib/FDBIndex';
import IDBCursor from 'fake-indexeddb/lib/FDBCursor';
import IDBDatabase from 'fake-indexeddb/lib/FDBDatabase';
import IDBTransaction from 'fake-indexeddb/lib/FDBTransaction';
import IDBObjectStore from 'fake-indexeddb/lib/FDBObjectStore';
import IDBRequest from 'fake-indexeddb/lib/FDBRequest';

global.indexedDB = indexedDB;
global.IDBKeyRange = IDBKeyRange;
global.IDBIndex = IDBIndex;
global.IDBTransaction = IDBTransaction;
global.IDBDatabase = IDBDatabase;
global.IDBCursor = IDBCursor;
global.IDBObjectStore = IDBObjectStore;
global.IDBRequest = IDBRequest;

const main = async () => {
    const db = await openDB("db", 1, {
        upgrade: (db) => {
            const foo = db.createObjectStore("foo", { autoIncrement: true });
            foo.add("hello");
            foo.add("goodbye");
        },
    });

    // Works:
    const tx = db.transaction(["foo"], "readwrite");
    const cursor = await tx.objectStore("foo").openCursor();
    cursor.delete();

    // Doesn't work:
    for await (const cursor of tx.objectStore("foo")) {
        cursor.delete();
    };
    await tx.done;
};

main();

Maybe something's not getting unwrapped properly somewhere?

@jakearchibald
Copy link
Owner Author

@quasicomputational thanks for spotting this. Reduced demo here https://static-misc.glitch.me/idb-lib-source-test/. I'll fix it.

@jakearchibald
Copy link
Owner Author

@quasicomputational fixed! Thanks for spotting.

@quasicomputational
Copy link

Looks good. I'm still getting one test failure, but it's a weird one and just the one - if there was a serious idb problem, I'd expect some of the related tests to be failing. I'll report back if I do track it down to idb, but thumbs up from me for the time being.

@quasicomputational
Copy link

I did just stub my toe on one thing: if, in one part of your code, you import from "idb" and another you import from "idb/with-async-ittr.js", the former can't unwrap the latter. The solution might be 'don't do that, then', but it's rather easy to do accidentally.

@jakearchibald
Copy link
Owner Author

@quasicomputational

you import from "idb" and another you import from "idb/with-async-ittr.js", the former can't unwrap the latter

I think that's a side effect of whatever build tool you're using. They should be the same, eg https://static-misc.glitch.me/idb-lib-unwrap-test/

Copy link
Contributor

@jeffposnick jeffposnick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just dropped a few suggestions for the README. Nice work!

README.md Outdated
```sh
npm install idb
```

Then in your JS:
Then, assuming you're using a module-compatible system (like Webpack, Rollup etc):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Webpack => webpack (if you care about their branding).

README.md Outdated
```sh
npm install idb
```

Then in your JS:
Then, assuming you're using a module-compatible system (like Webpack, Rollup etc):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you want to include a <script type="module"> example here, too? Someone wrote an explainer you could link to.

README.md Outdated
* `db`: An enhanced `IDBDatabase`.
* `oldVersion`: Last version of the database opened by the user.
* `newVersion`: Whatever new version you provided.
* `transaction`: The transaction for this upgrade. This is useful if you need to get data from other stores as part of a migration.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this a plain transaction, or a wrapped one?

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wrapped. Good point. I'll clarify.

README.md Outdated

`upgradeCallback` is called if `version` is greater than the version last opened. It's similar to IDB's `onupgradeneeded`. The callback receives an instance of `UpgradeDB`.
Transactions have a `.done` promise which resolves when the transaction completes successfully, and otherwise rejects.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm assuming that if it rejects, it will reject with the error value from https://developer.mozilla.org/en-US/docs/Web/API/IDBTransaction/error? Can you link to that for context?

README.md Outdated

* `abort` - as `idbTransaction.abort`
* `objectStore` - as `idbTransaction.objectStore`, but returns an `ObjectStore`
This is very similar to `localStorage`, but async. If this is *all* you need, you may be interested in [idb-keyval](https://www.npmjs.com/package/idb-keyval), you can always upgrade to this library later.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a comma splice here. You can switch out the , for a . preceding the last clause.

README.md Outdated

## Cursor
```ts
import { openDB, DBSchema, IDBPDatabase, IDBPTransaction } from 'idb';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At first, I thought the P in those class names was a typo. Maybe mention the promise-ified class names earlier on, when you first describe the wrapping?

@quasicomputational
Copy link

Oh, that makes some sense. I'm running it through Jest, using babel-jest; it wouldn't surprise me if there were some infelicities there.

I managed to pare down the bug I'm seeing and I can't reproduce it in a browser. Here's my current reproducer; it hangs after printing 'Done!' twice. So, yeah, probably not a bug in idb, but rather a race condition or similar in fake-indexeddb that's been newly exposed.

@quasicomputational
Copy link

Nope, probably an idb problem after all: an even more cut-down example shows that it's something to do with how transactions are wrapped, no promises needed. It still doesn't reproduce in the browser, though.

@jakearchibald
Copy link
Owner Author

@quasicomputational https://static-misc.glitch.me/idb-lib-source-test/ - I'm getting five logs for each example, as expected. What am I missing?

@quasicomputational
Copy link

I couldn't get it to happen in the browser either. But the two variants reliably do different things for me when invoked with node -r esm and using fake-indexeddb to mock IndexedDB for that environment.

@jakearchibald
Copy link
Owner Author

Feels like a fake-indexeddb bug.

@jakearchibald
Copy link
Owner Author

@jeffposnick thanks for the feedback. Lots of good catches there.

@quasicomputational
Copy link

Oh, I think you're right about it being a fake-indexeddb bug. Cutting down idb has revealed some weirdness going on with event listeners. I'll sort it out over there.

Copy link

@philipwalton philipwalton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Look's pretty good! I left a few comments/questions in the README diff.

README.md Outdated
* `oldVersion`: Last version of the database opened by the user.
* `newVersion`: Whatever new version you provided.
* `transaction`: The transaction for this upgrade. This is useful if you need to get data from other stores as part of a migration.
* `blocked` (optional): Called if there are older versions of the database open on the origin, so this version cannot open.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be worth linking to the blocked event here, and possibly also mentioning that the open request won't be blocked if you close all open connections in their respective blocking callbacks. (That may be more appropriate in a different section of the docs, not sure).

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll add some links to MDN, but I want to avoid documenting IDB in this readme.

I'm working on a "How to use IDB" series of doc which explain how IDB works, using this library. Once that's done, I'll link to it from this readme. That way the reader can decide "I know IDB, I just want to know about this library" vs "Explain everything".

README.md Outdated
* `newVersion`: Whatever new version you provided.
* `transaction`: The transaction for this upgrade. This is useful if you need to get data from other stores as part of a migration.
* `blocked` (optional): Called if there are older versions of the database open on the origin, so this version cannot open.
* `blocking` (optional): Called if this connection is blocking a future version of the database from opening.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar to above, maybe worth mentioning the versionchange event here, and explaining that calling this.close() (assuming it's a real versionchange event?) can prevent blocking if it's safe to do so.

README.md Outdated
idbKeyval.set('foo', {hello: 'world'});
## `deleteDB`

Delete a database.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: I'd change "Delete" to "Deletes", so it describes what the method does.

README.md Outdated
```

### Upgrading existing DB
If for some reason you want to drop back into plain IndexedDB, give one of the enhanced objects to `unwrap` and you'll get the unmodified version back.
Copy link

@philipwalton philipwalton Mar 13, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar nit to above: I'd start with a description of what the method does and then follow it with the why.

Takes a promisified idb object and returns the plain IndexedDB object it's wrapping. This is useful if for some reason you need to operate on the IndexedDB object directly.

README.md Outdated
```

### Getting all
Use this to convert a plain IDB object to one enhanced by this library. This is useful if some third party code gives you an `IDBDatabase` object and you want it to have the features of this library.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar comment as above.

README.md Outdated
return tx.complete;
});
```
These methods depend on the same methods on `IDBObjectStore`, therefore `getKey`, `getAll`, and `getAllKeys` are missing in Edge as it lacks support.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Didn't v3 polyfill these to some degree? Do you think it's worth adding an additional export folks can use if they need Edge support?

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, it polyfilled getAll. I'm going to hold off on adding too much stuff just for Edge, especially since they're switching to Chromium.

This is a tiny library that mirrors IndexedDB, but replaces the weird `IDBRequest` objects with promises, plus a couple of other small changes.
# IndexedDB with usability.

This is a tiny (~1.13k) library that mostly mirrors the IndexedDB API, but with small improvements that make a big difference to usability.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did the size increase (up from ~0.7k) happen because of the addition of convenience methods? If so, do you think it'd be worth also having a version without them (if feasible)?

If the lib is smaller than the previous version, it's probably not a huge deal, but it could be nice for those who don't use indexes much.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's smaller than the previous version, so I'm not that bothered.

I went back and forth on this, but the db.get methods are so commonly useful, I think they're worth it. They are however defined as a plugin, so it's easy to do a separate build if there's demand.

@jakearchibald
Copy link
Owner Author

Ok, let's ship this. Thank you all!

@jakearchibald jakearchibald merged commit 8465292 into master Mar 15, 2019
@jakearchibald jakearchibald deleted the 4.0.0 branch March 15, 2019 09:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants