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

Promise redeclaration / async incompatible with Typescript 2.0 #315

Closed
cherrydev opened this issue Sep 20, 2016 · 19 comments
Closed

Promise redeclaration / async incompatible with Typescript 2.0 #315

cherrydev opened this issue Sep 20, 2016 · 19 comments

Comments

@cherrydev
Copy link

As of Typescript 2.0 (or possibly 1.9, I never tested it there) declaring a top-level let Promise = Dexie.Promise; results in:

Duplicate identifier 'Promise'. Compiler reserves name 'Promise' in top level scope of a module containing async functions.

While before, using async after redefining Promise was already only marginally useful with Dexie because the redefinition had to go all the way up the call chain to the top-level 'async', it looks like the Typescript team finally closed this hole that allowed this to work.

Furthermore, the other suggestion in the documentation to redeclare Promise in an inner scope doesn't work, resulting in IncompatiblePromiseError: Incompatible Promise returned from transaction scope (read more at http://tinyurl.com/znyqjqc).

It appears that redefining the global Promise constructor with Dexie.Promise works, but will make it impossible to localize the changes to just a particular module or global block of code and will completely break Angular2 (which relies on ZoneJS patching the native Promise implementation).

I'm a bit concerned about the long-term viability of Dexie relying on a custom Promise implementation to allow transactions to work. The apparent goal of the transaction system is to allow transactions to span transparently between blocks of code that have specific knowledge of Dexie and portions that don't but if at any point in that chain a Dexie Promise gets wrapped (Promise.resolve()) or aggregated (Promise.all()) in a native Promise, it breaks. Zone.js appears to offer everything that is needed for Dexie to implement transactions, but without having its own separate Promise implementation (instead it patches an existing Promise implementation).

I only have a limited amount of time within my own project to try to figure this out, but if there are, in fact, ways of getting Dexie transactions to span multiple modules in the way that I've described, using Typescript 2.0, ES7-style async/await and Angular 2, I'd be very, very interested in seeing a demonstration of it. For now, I appear to be limited to using Dexie without async/await and without having any Dexie operations cross any Promise wrapper/aggregation boundary.

@dfahlander
Copy link
Collaborator

I was surprised by this. Most because I've interpreted the TC39 asyncawait proposal referring to NewPromiseCapability, which states:

The abstract operation NewPromiseCapability takes a constructor function, and attempts to use that constructor function in the fashion of the built-in Promise constructor to create a Promise object and extract its resolve and reject functions.

This sounded clearly that it's not tied to global Promise and that's why Typescript's breaking change surprised me a lot when they refer to the ES7 approach.

However, after digging more into the TC39 proposal, they actually refer to %Promise%, which is hard-referred to the native Promise. Either I've misread it before, or it has changed from being promise agnostic to become tied to native promises. The proposal went stage 4 (finished) as of July this year it has to be accepted. I tried my best last year to enter the discussions and propose a Promise-agnostic asyncawait but it didn't bite.

This means it is not only a typescript issue but also a ES7 issue and I will need to update the Dexie samples for ES7 and typescript since they do not apply.

I will have to implement support for native promises with Dexie. But the incompatibility between indexedDB and Promise in all but chrome browsers is still an issue. Hopefully this is an overgoing issue. As long as that is the issue, typescript 2.0 developers would have to stick to non-async semantics with Dexie, or target only Promise/IDB compliant browsers. ES6 developers could use yield instead of await.

@dfahlander
Copy link
Collaborator

I'm not saying this is the best solution, but it should be possible to replace window.Promise with Dexie.Promise before Zone.js starts wrapping stuff. If you try this out, I believe you would need to include dexie.js as a plain script tag before anything else, and set window.Promise = Dexie.Promise. Then include zone.js and the rest.

Long term code sustainability

When reading the Typescript issues about this, it seems they plan to loosen this strategy in a year or two. So it seems this will probably be possible in a future version of Typescript as long as they continue transpiling async functions to spawn/yield.

If there will come a day when Typescript stops transpiling async function (when asyncawait is natively supported across all browsers), we could get into a problem if indexedDB is still incompatible with native promises at that time. You would also be dependent on that Dexie, at that time, will adapt to native Promises, which I have a plan to do.

Summary

Two things itches with async / await:

  1. Replacing window.Promise with Dexie.Promise is an ugly solution, even though it should work out technically if doing it before Zone.js kicks in.
  2. Long term code sustainability is at risk because browser-native async/await is tied to native Promise which means you'll be depending on two things: A) Browser's incompatibility between native Promise and indexedDB #317 is resolved in Dexie. B) IndexedDB across all your target browsers will start to play ball with native promises.

Due to these questions, it is probably better to stop using async / await and go back to promise-style database code within transactions.

@cherrydev
Copy link
Author

angular/zone.js#455 indicates to me that your proposed solution won't work since if that approach doesn't work with Bluebird, I don't see it working with Dexie's promises either.

@dfahlander
Copy link
Collaborator

Aha, so zones replaces Promise entirely... Didn't know. But I'm not sure that has to be a problem since Dexie will only use Dexie.Promise internally which is not replaced.

Anyway, the ES7 async await when implemented natively won't respect changing window.Promise according to the spec. It'll still use the native Promise no matter. And as I understand it, there will be no way to hook in to the asynchronous callback when an await returns so zonejs won't be able to do its job either. I might be wrong but this is how I interpret the spec. Would be interesting to test with the Edge browser that has already implemented async await natively.

@cherrydev
Copy link
Author

While I won't pretend this is a completely sound argument, I very strongly believe that somehow, Google has the clout to ensure that Angular2 (which is fundamentally dependent on the concepts that zones provide) will not be left out in the cold when the ES7 spec solidifies. If this were my project, I would feel confident that a Dexie transaction implementation that was built on top of zones instead of custom Promises would be a reasonable long-term risk since the implications of ES7 fundamentally breaking zones would be so big.

And yes, I had already tried globally replacing Promise with Dexie.Promise and Zone.assertZonePatched caught me red handed.

Btw, Zones are already a proposed (stage-0) addition to ES7 meaning the exact semantics of ES7 async/await might be completely moot on that matter. https://github.com/tc39/proposals/blob/master/stage-0-proposals.md
It looks like the ES7 Zone proposal is ready to present to the committee but has not done so yet.

@dfahlander
Copy link
Collaborator

I see what you mean. However, I would not want to depend on zonejs. But I could consider ride upon it as an option. But still, indexedDB and native Promise does not cope anyway for the coming years in Firefox, ie, Edge and Safari so Dexie.Promise will still be need even if I use zonejs for keeping the reference to the transaction.

dfahlander added a commit that referenced this issue Sep 30, 2016
… replaces window.Promise withing the transaction scope only. NOTE: It replaces window.Promise temporarly only for code that executes within the transaction scope, so it won't disturb neither Zone.js or other libraries that depend on window.Promise. This will only affect your DB-facing code and code you call from withing your transaction scope.
@cherrydev
Copy link
Author

I've just started on a sabbatical of sorts for at least the next month, so I'm not able to follow up on this for quite some time. Good luck with it, though and I'll be in touch if I get back to this portion of the project.

dfahlander added a commit that referenced this issue Oct 7, 2016
… replaces window.Promise withing the transaction scope only. NOTE: It replaces window.Promise temporarly only for code that executes within the transaction scope, so it won't disturb neither Zone.js or other libraries that depend on window.Promise. This will only affect your DB-facing code and code you call from withing your transaction scope.
dfahlander added a commit that referenced this issue Oct 8, 2016
… replaces window.Promise withing the transaction scope only. NOTE: It replaces window.Promise temporarly only for code that executes within the transaction scope, so it won't disturb neither Zone.js or other libraries that depend on window.Promise. This will only affect your DB-facing code and code you call from withing your transaction scope.
@dfahlander
Copy link
Collaborator

Dexie 2.0.0 beta should work with Typescript 2.0 async function within transactions.

npm install dexie@next

@dfahlander
Copy link
Collaborator

Confirmed that it works now with Typescript 2.1:

https://github.com/dfahlander/Dexie.js/tree/master/samples/typescript

@wwoods
Copy link
Contributor

wwoods commented Oct 22, 2016

Testing this out with Typescript 2.1 / Dexie 2.0.0 beta.

I'm not sure if I'm calling the transaction wrong or not, but I keep getting "TransactionInactiveError: Transaction has already completed or failed" when using async functions with Dexie.transaction; my code is something like below:

let d = new Dexie("Test");
//init, set up tables...
d.transaction('r', d.table, async () => {
  //Not sure if this is significant, but in my code I await on an fs routine before dealing with db
  await fs.open(...);
  await fs.write("header");
  for (let table of d.tables) {
    await table.each(async (o) => {
      await fs.write(JSON.stringify(o));
    });
  }
});

Am I using the function wrong? If not, I can try to make a self-contained .ts script that reproduces the error.

@dfahlander
Copy link
Collaborator

You need to transpile to target es5 and don't involve babel, see the updated typescript sample in the repo.

@dfahlander
Copy link
Collaborator

Ok, I just read your code more carefully. It fails due to calling other async api within transaction. See https://github.com/dfahlander/Dexie.js/wiki/Best-Practices#3-dont-use-other-async-apis-inside-transactions

@dfahlander
Copy link
Collaborator

Tips is to call the other api before entering the db transaction

@wwoods
Copy link
Contributor

wwoods commented Oct 22, 2016

Oh it's an indexed db thing? That's really strange... Is there any sort of noop Dexie could do for the user when the promise hasn't returned? Something to keep the transaction alive? How is a tick defined here, just as one db operation?

If nothing can be done, I would suggest maybe putting a quick message about not missing a tick into the error message for a closed transaction.

@wwoods
Copy link
Contributor

wwoods commented Oct 22, 2016

For example, since several db promises can run in parallel, would it be possible to, when a promise is seen to be the result of the scopeFunc, to spawn a secondary promise that constantly queries nothing... Or iterates table names... Or something. Then have that promise loop until a variable is flipped on completion of the scopeFunc promise.

Since such an approach might affect performance, maybe have a flag in transaction() or a differently named transaction function altogether for this functionality?

Regardless of fs in my case, I'm sure other users will want to read/write in a transaction and make URL calls in between based on data read. As you said, it's best to do so outside of a transaction, but not always possible. e.g. for dumping the database in a consistent state (my use case). An officially supported workaround would be very nice.

@wwoods
Copy link
Contributor

wwoods commented Oct 22, 2016

I see. Looks like each promise has to have activity each tick to not get shut out of the database. The Table.each() promise would never return in the non-busy thread. Annoying, but not much you can do. Thanks for the help!

A note for anyone reading this hoping for a workaround: I spawned a secondary async method using a done flag shared between the async in the transaction and this async. Wrapped the transaction in a try..finally, and in finally set done = false and wait on the secondary async method that handles file IO. They communicated over an array that the secondary method would "steal" in its processing loop.

@dfahlander
Copy link
Collaborator

It would be possible to implement readonly db snapshots that persist over transactions, but it would be cumbersome. (using crud hooks that record changes and emulate db api).

There is also a new proposal going on to enhance indexedDB with a IDBTransaction.waitUntil(promise). Would solve the issue. But it's far future :(

@dfahlander
Copy link
Collaborator

Using noop would be possible as well but it would have to be flagged for due to performance reasons.

@wwoods
Copy link
Contributor

wwoods commented Oct 23, 2016

Just to clarify, I tried having a noop thread work alongside the thread that was alternating fs and Dexie.Table calls. Although Dexie didn't raise an error anymore, the code did not work either, presumably because each promise "thread" needs to have indexedDB activity each tick to stay active. That's why I changed to two promise threads, a file writer and a db reader. The db reader just reads as fast as it can, and the file writer checks the buffer periodically and swaps it out / writes what was read to disk. It's effective, but not generalizable to a single async call with both non-Dexie and Dexie awaits.

So, I don't think the noop is feasible, unless you got really fancy and replaced all of Dexie's methods with versions that basically filled a work queue for the noop thread, which then ran those methods. Sounds very obfuscated and slow.... but I guess it would work.

Then again - if you're making a new transaction() function anyhow, I suppose that function could pass a proxy object as the first parameter, and then the transaction as the second. It would then be the user's responsibility to use the proxy object rather than the original database. Certainly a safer, albeit still confusing, option.

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

No branches or pull requests

3 participants