Fix #856: Be more lenient detecting window.fetch, to support polyfills. #964
Conversation
| var hasFetch = | ||
| typeof fetch === 'function' && | ||
| fetch.toString().indexOf('[native code') !== -1; | ||
| var hasFetch = typeof fetch === 'function'; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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. 🙂
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Just noting that I've not forgotten this PR and I think it's the right approach, but see my comment about it being a breaking change regarding compatibility: #964 (comment)
I think this is worth doing, but let's do it in a future major version.
|
BTW I just came across this PR https://github.com/localForage/localForage/pull/799/files#diff-772d99c4330eadb13238eb2709bc7731 which appears to have fixed this issue in the past. This makes me wonder if and/or why it got rolled back? |
|
Any plans to tie this PR off any time soon? |
|
Bump? Sorry to be a pain. This is still a blocking issue for us. |
|
I'm also running into this issue. Bugsnag.js SDK monkey patches fetch() for logging breadcrumbs. When emulating iOS on desktop Chrome browser, it switches to WebSQL. The result is that it uses a different database than when either not emulating or when emulating Android. It would be great if there were a config option to bypass the fetch() check, or to just force a particular driver. It looks like the source code hides all possible monkey patches inside closures, and I'd rather not fork this repo. And the config API rejects specifying drivers that are not "supported". |
Fixes #856