-
-
Notifications
You must be signed in to change notification settings - Fork 642
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
Comments
I was surprised by this. Most because I've interpreted the TC39 asyncawait proposal referring to NewPromiseCapability, which states:
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. |
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 sustainabilityWhen 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. SummaryTwo things itches with async / await:
Due to these questions, it is probably better to stop using async / await and go back to promise-style database code within transactions. |
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. |
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. |
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 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 |
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. |
… 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.
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. |
… 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.
… 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.
Dexie 2.0.0 beta should work with Typescript 2.0 async function within transactions.
|
Confirmed that it works now with Typescript 2.1: https://github.com/dfahlander/Dexie.js/tree/master/samples/typescript |
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. |
You need to transpile to target es5 and don't involve babel, see the updated typescript sample in the repo. |
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 |
Tips is to call the other api before entering the db transaction |
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. |
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. |
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. |
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 :( |
Using noop would be possible as well but it would have to be flagged for due to performance reasons. |
Just to clarify, I tried having a noop thread work alongside the thread that was alternating So, I don't think the noop is feasible, unless you got really fancy and replaced all of Then again - if you're making a new |
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.
The text was updated successfully, but these errors were encountered: