indexeddbshim / IndexedDBShim

A polyfill for IndexedDB using WebSql
Other
968 stars 191 forks source link

Which Browsers and Versions Are Targeted? #330

Closed StabbarN closed 5 years ago

StabbarN commented 6 years ago

I'm trying to understand what browsers this shim supports and how to adjust targets for polyfilling. We can't import babel-polyfill before indexedDBShim (as suggested in readme). For example, has it been made sure that this shim is polyfilled for ios9 or later?

In Gruntfile.js I found

const babelBrowserOptions = {
    sourceMapsAbsolute: true,
    plugins: ['add-module-exports'],
    presets: ['env']
};

and changed it to

const babelBrowserOptions = {
    sourceMapsAbsolute: true,
    plugins: ['add-module-exports'],
    presets: [
        ['env', {
            targets: {
                chrome: '49',
                ios: '9'
            }
        }]
    ]
};

I also added import of babel-polyfill to src/browser.js:

/* eslint-env browser, worker */
import 'babel-polyfill';
import setGlobalVars from './setGlobalVars';
import CFG from './CFG';

CFG.win = typeof window !== 'undefined' ? window : self; // For Web Workers
setGlobalVars();

This seems to work except on ios9. Tested on ios9, ios10, ios11 and latest chrome. On ios9 error Error: SyntaxError: Unexpected token '[' on a long row where the shim is imported.

Do you have any idea what the problem may be or can suggest a better way?

StabbarN commented 6 years ago

Adding import 'babel-polyfill'; as above and not setting targets seem to be OK (it runs fine on ios9).

brettz9 commented 6 years ago

Hi @StabbarN ,

Re: preset env, I don't know why I didn't consider that before! And I have been just preparing a commit to add eslint-plugin-compat which also uses browser versions but to alert one to limitations of browsers against one's current code, but we also need to ensure the ES features lacking in those browsers are converted by Babel.

I guess the reason I haven't focused on this, has been that my focus has been on adding Node support and ensuring better compliance against IndexedDB with our internals. I also haven't updated some earlier code we had which partially polyfilled some of old Safari bugs or what not. (IIRC, one can't completely replace with one's own indexedDB implementation in some of these browsers.) Anyways, it'd be great if this is enough to get things working (and some other similar bugs are still open which may be fixed by this). But there may still be some subtle problems that proper Babelification alone won't be enough to fix. I'd be happy to try to help or answer questions, but as mentioned, my own focus is on Node and internals rather than specific browser environments.

As far as import 'babel-polyfill', I don't want to add it to source because other libraries may be bundling IndexedDBShim and they may have their own use of this file. Also, some environments may not need all of that polyfill code. The import really only fits if added to say the main entrance of your web application (and if doing it as an import, you'd need it as a full path and it would only work in browsers supporting ESM).

Most IndexedDBShim users will probably instead want to add it as a regular script tag (at least conditionally), as we indicate on our README, so things can work in older browsers. While it may seem less than ideal to "pollute" one's HTML in this way, since the code is polyfilling standard behavior, I believe it actually makes the most sense to put it there, as this is a global dependency (and one which could be dropped later), not a modular one (where the dependencies in such cases indeed ought not be in the HTML outside of their context).

That being said, I do hope to convert to Rollup to allow a full ES6 distribution in the future, so if only supporting modern browsers, one can use modules directly (including import 'babel-polyfill' at the top of the main entrance if still needed by those browsers for other ES features they don't yet support). This would probably look like import setGlobalVars from './node_modules/indexeddbshim/dist/indexeddbshim-noninvasive-es.js'; and we could probably have a ponyfill version as well import {indexedDB, IDBKeyRange} from './node_modules/indexeddbshim/dist/ponyfill-es.js';. See also #283.

brettz9 commented 6 years ago

I'll try to prepare a fix for the preset-env issue shortly. You should also know that I'm planning a new major release which has some breaking changes, including with babel (since I'm upgrading to Babel 7, its new npm-namespaced structure as @babel/polyfill (which also changes folders in node_modules to use a subfolder structure) instead of babel-polyfill will necessitate a path change to this dependency).

brettz9 commented 6 years ago

Apologies for the delay in getting to the fix--maybe a few days more as want to get a few small things off my plate first... Thanks!

StabbarN commented 6 years ago

As far as import 'babel-polyfill', I don't want to add it to source because other libraries may be bundling IndexedDBShim and they may have their own use of this file. Also, some environments may not need all of that polyfill code. The import really only fits if added to say the main entrance of your web application (and if doing it as an import, you'd need it as a full path and it would only work in browsers supporting ESM).

Most IndexedDBShim users will probably instead want to add it as a regular script tag (at least conditionally), as we indicate on our README, so things can work in older browsers. While it may seem less than ideal to "pollute" one's HTML in this way, since the code is polyfilling standard behavior, I believe it actually makes the most sense to put it there, as this is a global dependency (and one which could be dropped later), not a modular one (where the dependencies in such cases indeed ought not be in the HTML outside of their context).

Adding 'babel-polyfill' as a script tag affects the whole project and afaik no other frameworks that we use have this approach. For example, what if the project isn't using babel as a transpiler or what if transpiling to a defined browser list? Should the whole project be bound to the babel-polyfill that this lib requires?

IMO, it would be great if this shim build dists that include everything needed to use it in the explicitly supported environments without having constraints on polyfills on the rest of the project.

brettz9 commented 6 years ago

The docs for https://babeljs.io/docs/en/next/babel-polyfill.html indicate:

Babel includes a polyfill that...is intended to be used in an application rather than a library/tool

Also per https://babeljs.io/docs/en/next/babel-preset-env#usebuiltins

NOTE: Only use require("@babel/polyfill"); once in your whole app. Multiple imports or requires of @babel/polyfill will throw an error since it can cause global collisions and other issues that are hard to trace. We recommend creating a single entry file that only contains the require statement.

The polyfill, moreover, is fairly large, so even if some want it, others might not, and one certainly wouldn't want multiple copies, even if one works around the fact that it may err upon encountering duplicates...

If you don't want a script tag, you can use an import statement for the polyfill and bundle the JavaScript containing the import statement.

Consuming projects do not at all need to use Babel to transpile... Our dist files do that for you... @babel/polyfill deals only with polyfilling globals and such--the ES6 syntax and what not, is handled instead by @babel/core (and @babel/preset-env) which we use in our build process.

I'm not sure how the requirement to add babel-polyfill (or soon @babel/polyfill) adds constraints to polyfills for the rest of the project. As a polyfill, it should behave like the native features of the browser, so a polyfill is not interfering with other projects unless there is some bug in the polyfill implementation (which should be reported).

If in speaking of "transpiling to a defined browser list", you are concerned about adding undue weight with the inclusion of all of @babel/polyfill, this concern has merits, but not bundling it at least allows for one to choose the specific polyfills used. If your project does choose Babel, you can avoid @babel-polyfill by importing IndexedDBShim into your code along with one @babel/polyfill statement and the option usebuiltins which, in conjunction with @babel/preset-env, will replace the whole polyfill inclusion with only those individual polyfill items needed by your targets...

I'm not intending to stake out an inflexible position here, so feel free to continue with counterpoints or alternative suggestions... But as I see it now, our current approach makes the most sense...

StabbarN commented 6 years ago

Thank you for taking this issue seriously. We are very grateful for this library.

I'm out on deep water and learned a lot abut how packages should be published in the last days :)

We were out looking for an explanation of a strange behavious and thought that a potential bug had occurred because we use @babel/polyfill and that this shim only worked with babel-polyfill. We have probably found the bug and I will file another issue about that one when we are certain.


For clarity, why is babel-polyfill declared as a dependency and not devDependency? Do you need it as a dependency for the node set-up?

We use @babel-polyfill (not babel-polyfill) but that dependency generates a yarn.lock which includes an unused babel-polyfill:

...
babel-polyfill@6.26.0:
  version "6.26.0"
  resolved "https://registry.yarnpkg.com/babel-polyfill/-/babel-polyfill-6.26.0.tgz#379937abc67d7895970adc621f284cd966cf2153"
  dependencies:
    babel-runtime "^6.26.0"
    core-js "^2.5.0"
    regenerator-runtime "^0.10.5"
...
indexeddbshim@3.10.0:
   version "3.10.0"
   resolved "https://registry.yarnpkg.com/indexeddbshim/-/indexeddbshim-3.10.0.tgz#3cb9071d831ad0ae567fedd4dddcf346d3654ed7"
   dependencies:
     babel-polyfill "6.26.0"
...

To reduce pollution in our case, you could set it as a devDependency and then in the readme state something like (1) "Run npm install babel-polyfill, (2) import or include it as a script tag.".

brettz9 commented 6 years ago

Very glad to hear you find it useful...

As far as babel-polyfill vs. @babel/polyfill, the latter was used starting with Babel 7 (so as to namespace all Babel packages within the Babel namespace for clear ownership and also impacting storage within node_modules, adding polyfill as a subfolder of babel along with any other babel packages).

I hope to switch us to Babel 7 and thus to @babel/polyfill, necessitating unfortunately a breaking change, as it changes the node_modules path. However, I have some changes to work on for our dependency typeson-registry to get it using @babel/polyfill and hopefully to see about why its latest incarnation (which we are not yet using), which would be expected to be an improvement, is causing some W3C test failures for us as far as Blobs.

Some other breaking changes are expected:

  1. Removing Bower (service was deprecated, so probably best with the breaking changes to force this change now too)
  2. To continue to work with data stored under 3.*, users will need to add a callback as the value of the new CFG.registerSCA setting with a value set to the function exported from typeson-registry-sca-reverter--code which was formerly within IndexedDBShim, and do so before you make your indexedDB calls. This code--which converts from the old Typeson Structured Cloning Algorithm (SCA) format into a more recent one is no longer bundled, as IndexedDBShim will instead use the new typeson-registry format by default for representing cloneable data as JSON. Though it worked, the old format was problematic as a format as it would require renaming upon importing to avoid clashing with the builtins it was serializing/deserializing. But IndexedDBShim old data should still work as long as the config is set with that callback--and I don't expect removing that callback in the future.
  3. Disallow deprecated numeric constants to IDBDatabase.prototype.transaction as second argument (to use "readonly"/"readwrite" instead).
  4. Remove our non-standard IDBFactory.modules (non-IDB shims or polyfills available through CFG.addNonIDBGlobals or CFG.replaceNonIDBGlobals, respectively, with shims prefixing class names with "Shim")
  5. Remove our non-standard IDBFactory.utils and move its test utility createDOMException to become (probably) a named export of setGlobalVars.js

Yes, we include babel polyfill as a dependency so that it is available out of the box to users to include with their script tags or imports. Hopefully the pollution will be reduced now for more users by switching to Babel 7. I hesitate to make things more difficult for web use, as I think such use should be the easiest. A pity optionalDependencies are not (yet?) truly so...

brettz9 commented 5 years ago

It took a while, and there were a few more breaking changes, but 4.0.1 is now released, and you might try against it...

StabbarN commented 5 years ago

Looks great, thank you!

brettz9 commented 5 years ago

@StabbarN : As an FYI, the new 5.0.0 release now removes the dependency on the now deprecated @babel/polyfill in favor of what they recommend--and what @babel/polyfill has had as dependencies--core-js-bundle and regenerator-runtime.

I've considered whether, as a library, we might just leave it up to applications to add these, and remove them from our dependencies, but I think we'd need regenerator-runtime anyways...

StabbarN commented 5 years ago

Thank you for spreading the info. We are, however, only using this lib on older browsers so we are very cautious about upgrading to 5.0.0.