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

Making the indexeddb adapter publicly consumable #7618

Merged
merged 9 commits into from Aug 14, 2019

Conversation

SCdF
Copy link
Contributor

@SCdF SCdF commented Jan 24, 2019

It's time to take indexeddb out from under NEXT=1 and into the spotlight. Or at least the npm install light..

This PR is attempting to do such a thing. I am probably missing steps. For now I'm creating this PR so travis runs tests, as it seems profoundly disinterested in this branch otherwise.

Things done in this PR:

  • Trying to open an idb-created DB with the indexeddb adapter errors
  • Trying to open an indexeddb-created DB with the idb adapter errors
  • Removed the private flag from the adapter, which I presume is all I need to do to enable eventual npm publishing
  • Removed the NEXT=1 stuff and replaced it with referencing the adapter directly
  • Unfortunately massively increased the number of tests travis runs.

I'm not sure what to do about that last one. We really should be testing each supported adapter in each supported browser, but that's a matrix of doom. This will drop down once we drop / deprecate idb support, but there are still lots of combinations we're not testing here (eg most of Safari).

@SCdF SCdF force-pushed the make-indexeddb-public branch from a8aae9e to 010ad5e Compare Jan 26, 2019
@SCdF SCdF force-pushed the make-indexeddb-public branch from 573f74e to ea4f974 Compare Feb 18, 2019
@stale
Copy link

@stale stale bot commented Apr 22, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the wontfix label Apr 22, 2019
@SCdF
Copy link
Contributor Author

@SCdF SCdF commented Apr 22, 2019

No we definitely want to do this, I just need to not be crushed at Normal Work™...

@stale stale bot removed the wontfix label Apr 22, 2019
@alexfedoseev
Copy link

@alexfedoseev alexfedoseev commented May 6, 2019

@SCdF I'm ~1-2 weeks away from initial release and I'm not sure if I should switch to the new adapter or stick with the current for now. AFAIU those are not compatible and there's no way yet to easily upgrade dbs on switch. What would you recommend?

@SCdF
Copy link
Contributor Author

@SCdF SCdF commented May 7, 2019

@alexfedoseev for now I wouldn't, though I would greatly appreciate any testing you want to perform!

Unfortunately getting this merged is a secondary concern for me as there has been some Super Urgent Critical™ stuff at work for the past few months that has reduced my ability to contribute here. I'm hoping in a month or so I can get back on track and find the time to fix the build and get this work moving again.

On the migration, worst case the recommendation would be to replicate between the two to perform the migration, but I'm hopeful that it's not that much pain to just alter the internal structures. I haven't looked into it deeply yet.

@alexfedoseev
Copy link

@alexfedoseev alexfedoseev commented May 8, 2019

@SCdF Thanks for the feedback! I will stay on the current version then. I’m not familiar with the way pouchdb gets built. If you can give me some pointers how can I consume next adapter, I’ll check it out sometime. Api is the same, right?

@SCdF
Copy link
Contributor Author

@SCdF SCdF commented May 8, 2019

@alexfedoseev unfortunately it's hard to use the new adapter right now (this is what this PR is meant to solve), you need to basically use the hidden NEXT=1 build, which I do not believe is possible via standard npm-based imports.

@garrensmith garrensmith mentioned this pull request Jun 13, 2019
4 tasks
SCdF added 6 commits Jun 17, 2019
idb can't open indexeddb dbs because the version is too old. We detect
that and generate a useful error to the user, which happens when you
try to instantiate the handle (new PouchDB(..))

for indexeddb we check to see if the old version is in the range that
idb occupies but indexeddb never does. Unfortunately due to indexeddb
lazily opening handles this only happens once you actually perform
an action on the DB (dbhandle.post({foo: 'bar'}))
@SCdF SCdF force-pushed the make-indexeddb-public branch from 9d1704c to c340d92 Compare Jun 17, 2019
@SCdF SCdF requested review from daleharvey and garrensmith Jun 18, 2019
@SCdF
Copy link
Contributor Author

@SCdF SCdF commented Jun 18, 2019

@daleharvey / @garrensmith so I think this is ready to look at. I have no idea what I'm doing, and I don't actually know if this will expose this adapter on npm once a publish kicks off.

I also fixed the Chrome tests, and they now run on Saucelabs. This change contains a very large travis yml, which basically tests everything on both adapters (new and old) as well as on Chrome and FF. I think at a minimum we should keep that latter change (everything on Chrome and FF) since they are both important browsers.

// adapter?
// Note that we're no longer invoking that adapter by passing NEXT=1, so
// this code isn't firing. If these tests work with indexeddb builds on travis
// we can drop this.
Copy link
Member

@daleharvey daleharvey Jul 20, 2019

Choose a reason for hiding this comment

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

Yup lets drop

@daleharvey
Copy link
Member

@daleharvey daleharvey commented Jul 20, 2019

Sorry for the long long delay here, in future feel free to 'timebomb' these commits, ie if you dont get a timely review (2 weeks) and have commit access, feel free to land

So I think this is very nice, we can drop the autoCompaction thing as you mention, the one issue is I would like to avoid adding a whole bunch of new test runs, especially if we need to move chrome to saucelabs for now

We dont need (and will never get) full coverage, while indexeddb is still optional, making sure it works in firefox + chromium + node is fine, then as we switch it to be the default it can get full browser coverage

We can take out the AUTO_COMPACTION runs completely at this point since nobody has touched that code in a long time

@daleharvey
Copy link
Member

@daleharvey daleharvey commented Aug 14, 2019

Tell you what, lets merge this and can drop the uneeded test runs in a follow up

@daleharvey daleharvey merged commit 3ed65f3 into master Aug 14, 2019
2 checks passed
@SCdF
Copy link
Contributor Author

@SCdF SCdF commented Aug 14, 2019

(Hey! Sorry I was so delayed following up with this, I've been travelling for work and hadn't found time to read your comments yet. Thanks for the merge!)

@SCdF SCdF deleted the make-indexeddb-public branch Aug 14, 2019
@garethbowen garethbowen mentioned this pull request Dec 4, 2019
1 task
sto3psl pushed a commit to sto3psl/pouchdb that referenced this issue Mar 10, 2021
* Remove the private flag to we publish this adapter to npm

* Stopping idb and indexeddb from opening each other

idb can't open indexeddb dbs because the version is too old. We detect
that and generate a useful error to the user, which happens when you
try to instantiate the handle (new PouchDB(..))

for indexeddb we check to see if the old version is in the range that
idb occupies but indexeddb never does. Unfortunately due to indexeddb
lazily opening handles this only happens once you actually perform
an action on the DB (dbhandle.post({foo: 'bar'}))

* Removing NEXT generation

* Apparently my editor's eslint plugin is broken again..

* Update travis to also test indexeddb adapter

* Does changing all chromes to saucelabs magically work with no additional effort?

* Dropping chrome_bin

* More debugging on selenium / sauce connections

* Removing unneeded logging
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.

None yet

3 participants