Refactoring Dexie

[chrahunt]

One thing that may make it difficult for contributors to Dexie is the relatively monolithic code base. Having tried to do some amount of work on Dexie in the past, it can be difficult to identify the scope and responsibilities of individual parts when everything is technically in scope.

Can we have a concerted effort to refactor and move classes out of the main Dexie function so it is more clear what components are more tightly coupled?

[dfahlander]

Agree. The code suffers from originally being written with heavily use of closures. I think it's time to refactor it to use classes.

[benitogf]

this would be very nice 👍

[chrahunt]

Here's what I've got down so far. This can be done over several PRs to ensure progress and reduce risk:

• [ ] replace utility functions with modern equivalents - reduces boilerplate imports in each class

  • [ ] getProto: Object.getPrototypeOf
  • [ ] keys: Object.keys
  • [ ] isArray: Array.isArray
  • [ ] hasOwn: Object.hasOwnProperty
  • [ ] defineProperty: Object.defineProperty
  • [ ] asap: ensure compiler provides setImmediate instead
  • [ ] shallowClone: Object.assign({}, obj)

• [ ] Consolidate indexedDB utilities/constants into separate file - central location for use by other classes

  From utils:

  • getByKeyPath
  • setByKeyPath
  • delByKeyPath
  • minKey
  • maxKey

  from Dexie:

  • cmp
  • min
  • max
  • ascending
  • descending
  • READONLY
  • READWRITE
  • safariMultiStoreFix
  • maxString
  • minKey
  • maxKey
  • adjustToExistingIndexNames
  • hasGetAll: also move assignment from adjustToExistingIndexNames
  • getNativeGetDatabaseNamesFn

• [ ] Move functions used in a single context or move to more descriptive namespace

  from Dexie:

  • tempTransaction: move to Table, only used there
  • preventDefault: move to Events, then use elsewhere
  • simpleCompare: move to WhereClause, only used there
  • simpleCompareReverse: move to WhereClause, only used there
  • parseIndexSyntax: add as static method on IndexSpec, then use elsewhere
  • readGlobalSchema: move most of function to static method on TableSchema
  • getSchemaDiff: add as static method on TableSchema

  from utils:

  • getArrayOf (+NO_CHAR_ARRAY, iteratorSymbol, getIteratorOf): move to WhereClause, only used there
  • trycatcher: move to Collection, only used there

With those done, the following dependencies will still exist:

• Transaction

  • db: may be made available via constructor (interface change)
  • idbdb: add as property of db and mark as private API
  • allTables: property of db (_allTables)
  • dbOpenError: add as property of db and mark as private API

• Table

  • db: may be made available via constructor (interface change)
  • allTables: property of db (_allTables)

Once those interface changes are done then all classes may be moved from Dexie and then refactored to use modern constructs:

• [ ] Classes moved from Dexie body into separate files

  • [ ] Collection
  • [ ] Table
  • [ ] Transaction
  • [ ] WhereClause
  • [ ] IndexSpec
  • [ ] TableSchema

• [ ] For each class, move 'private methods' out of props callback into top-level

• [ ] For each class, function -> class

• [ ] function-level code moved to constructor

• [ ] public methods moved from return value to props callback to class body

Please comment on any items and I can update as appropriate before starting.

[nponiros]

Would it make sense to start writing things in typescript during the refactoring? Typescript is also mentioned in the vision for dexie issue (https://github.com/dfahlander/Dexie.js/issues/427). Maybe some other things from there are relevant for the refactoring.

We should also consider browser support when using modern constructs. Do we offer multiple bundles for modern and older browsers? Who adds polyfills? The user or dexie?

We would also need to consider breaking changes. It is probably safer to release v3 with so many changes. Even if not everything is done in one go, just replacing functions (example shallowClone vs. assign) might break things.

[chrahunt]

Would it make sense to start writing things in typescript during the refactoring?

I consider this issue resolved when the classes that comprise Dexie are in separate files and all tests pass. I think this is a pre-requisite for several changes like those mentioned in #427, but there's no need to address those here.

Do we offer multiple bundles for modern and older browsers? Who adds polyfills? The user or dexie?

The current distribution method (single package for all users) should be maintained. Polyfill was the wrong word, I meant that the compiler or a plugin should provide an implementation that gracefully handles the case when a native version doesn't exist.

It is probably safer to release v3 with so many changes. Even if not everything is done in one go, just replacing functions (example shallowClone vs. assign) might break things.

The majority of changes are private. The only publicly-visible items should be the interface changes noted for Transaction and Table, but those can be made in a backwards-compatible way.

[nponiros]

I don't think dexie should handle the case where for example Object.assign is missing (this is what I meant with polyfills, I'm not talking about syntax like class). Let the user decide if the browsers he uses need a polyfill. This also makes sure that polyfills are just imported once.

[dfahlander]

Refactoring out classes

We can start with refactoring out classes like Table, Collection, WhereClause etc into typescript classes. If not used to typescript, just start out with plain ES6. We already use tsc to transpile the JS code, so it's just to create a file table.ts and start refactoring the code in.

What needs to be taken care of are:

• Some closures used as private props today, need to be put as real props (maybe typescript-private and prefixed with underscore)
• Class constructors should be instance-local and put on db (db.Table, db.Collection, etc), as it keeps it possible for an addon to extend prototype only for db instances that use that addon. This can be accomplished by either copying the prototype, or extending it for each db instance.

ES5 compliance

I still think libraries are expected to work on ES5, even though the major browsers are on ES2016 already. IE11 will be a pain for some more years in the corporate world. This would force us to either stay away from Object.assign() or polyfill it in the transpilation process.

Minifaction size

Generally, I've been aliasing Object.getPrototypeOf(), Array.isArray etc for a reason - it makes the minified code much smaller. I could understand the readability issue though, but I'd rather see getProto() renamed to getPrototypeOf() - something that should be more readable for lots of people.

[dfahlander]

Should add that we should stay away from async/await or yield in the source code - also because we don't want the transpiled output to be too large. This will stay true as long as we need to transpile to ES5, which will be true as long as libraries are expected to be delivered in ES5. Time will come when that is not true anymore, but it's not yet.

[dfahlander]

I think we would be able to use importHelpers to polyfill missing ES5 parts including Object.assign() etc. Described in this blog. I suppose installing tslib and marking it as an external library with in rollup config would do it for us. Needs to be verified. Would be good because then we can feel free to use Object.assign() as well as spread operators on objects.

[dfahlander]

@chrahunt What's the status of your PR? Do you want to complete your PR before anyone of use starts the refactoring?

[nponiros]

Object spread does not strictly require Object.assign. In fact object spread and Object.assign are not equivalent. Object.assign calls setters whereas object spread does not.

I don't think adding polyfills is a good idea. It is better to just mention the needed polyfills as requirements and let the user add those. React, Angular and probably all other major libraries do the same. If we add the polyfills there is a big chance that the user will also add his/her own polyfills and there would be a bunch of duplicated code there. Also all modern browsers support ES2015 so adding a polyfill would only add more data to download for the browsers that don't need it.

[dfahlander]

Did some testing with the latest tsc. It seems tsc will transpile

const clone = {...orig};

...to use assign(), and inline a definition of assign var __assign = this.__assign || Object.assign || function (){...} in top of the ES5 output, as Object.assign() isn't part of the ES5 standard so it can't rely on it. It is possible to configure tsc to not do this with the option noEmitHelpers, but this will require our users to include tslib for dexie to work. I still lean to use importHelpers instead and let rollup do the treeshaking to only include the helpers that are being used, like __assign etc. The code will be small if we try avoiding all fancy stuff like async/await. Object and array spread could be very handy though and improve readability at some places.

Though if we want to use Object.assign() explicitely, tsc would leave it as is so we would require user to include polyfills. We could workaround that by using __assign() in place of Object.assign():

import { __assign } from 'tslib';

Rollup will fix it so that __assign is only defined once in dist/dexie.js.

[dfahlander]

I've started a limited refactoring work #622

[dfahlander]

Closing this as the discussion here is already implemented/refactored into typescript classes, interfaces and functions.