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.

Sign up for GitHub

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

Jump to bottom

Fix #856: Be more lenient detecting window.fetch, to support polyfills. #964

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Fix #856: Be more lenient detecting window.fetch, to support polyfills. #964

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 1 addition & 3 deletions src/utils/isIndexedDBValid.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,9 +17,7 @@ function isIndexedDBValid() {
!/Chrome/.test(navigator.userAgent) &&
!/BlackBerry/.test(navigator.platform);

var hasFetch =
typeof fetch === 'function' &&
fetch.toString().indexOf('[native code') !== -1;
var hasFetch = typeof fetch === 'function';
Copy link

Choose a reason for hiding this comment

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

I'm a bit worried that the entire point of checking for a native implementation was to ensure that the script's running in a browser with a native fetch because that's an indication that the given browser supports IndexedDb. This is also known as feature inference.

After we make this change, polyfilling fetch in any browser would trick localForage into thinking that IndexedDb is also supported which might not be the case.

Copy link
Member

Choose a reason for hiding this comment

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

That's quite true, though that code is also a reflection of the state of JS at the time.

Out-of-scope of this PR, I think it would benefit future versions of localForage to take a more "evergreen browser" support approach and leave the 1.x branch to maintain broader compatibility. The original two selling points for localForage were compatibility and ease-of-use, but the former is much less an issue over five years after its introduction. Effectively, browsers support IndexedDB and localForage exists to work around quirks and provide a nicer API.

I think we should make this change, but I'm thinking it might be best released in a new major version.

I've been trying to focus on localForage 2.0 for some time, but other things have consistently interrupted 😅. I think I should actually have some time for it this month, so I'll see what I can do. 🙂

Copy link

Choose a reason for hiding this comment

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

As an alternative workaround, we could tackle this problem similar to what jQuery.noConflict did.

We could write and document a tiny JS snippet that the user of this library would need to insert before the polyfill that would set a global variable on the window and we could use the global variable in this condition.

Thoughts?

Copy link
Member

Choose a reason for hiding this comment

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

My first reaction is that it sounds a bit complicated to explain, and could be a bit frustrating to implement as well. Of course, the tradeoff is it not working as-expected with a modified fetch, so fair enough. I'd, of course, prefer libraries weren't modifying built-in fetch, but I get why it might happen 😄

I'll think about the best way to address this and try to sort out timelines for breaking changes. I'd really like to start improving the internals of localForage and modernise the library, including removing much of the older compatibility hacks/fixes and the callback support for modern JS environments. That'd be the perfect time to introduce this fix, in my opinion, so if it isn't too late it's what I'd prefer.

But if that doesn't happen soon, then we should explore shipping this fix in a non-breaking way, even if it involves a type of noConflict route.

Copy link

Choose a reason for hiding this comment

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

I'd, of course, prefer libraries weren't modifying built-in fetch, but I get why it might happen.

Agreed. In our case, it's Fullstory and Sentry that both wrap the native fetch so that they can collect breadcrumbs. Not sure if there's an alternative approach so libraries will always do that.


// Safari <10.1 does not meet our requirements for IDB support
// (see: https://github.com/pouchdb/pouchdb/issues/5572).
Expand Down