-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Migrating Async/Await to Promise #403
Conversation
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit 473c16a:
|
Co-authored-by: Daishi Kato <dai-shi@users.noreply.github.com>
@dai-shi https://gist.github.com/barbogast/5679eabc25b761da0f500f53ca3ea0c2 contains an idea for a fix of the remaining issue with the rehydration. Is this approach correct, or do you think it should be fixed in an entirely different way? |
Just updated both gists. The tests should be finished, and are passing against the updated |
I'm wondering... is it possible to correctly type That together with the subtle but important differences between sync mode and async mode within the same code block make me wonder if it's worth it to handle both cases with the same code. Maybe it would be better to duplicate the code and handle both cases separately. Not sure, though, just sharing the thought. |
I am going to make some changes to add support for sync error handling. then you can make a PR on the repo |
Hi dear @dai-shi. |
the async tests are failing. dear @barbogast would you mind please check that. |
https://gist.github.com/TheEhsanSarshar/d9852a7cc102dae0f07186e1dc4fe700#gistcomment-3761417 |
FWIW, I created a PR with my results agains @TheEhsanSarshar 's branch: byteab#1. The changes are separated by commit, so you can cherry-pick. It contains the tests for the sync storage and code adjustments to |
Hmm... just realised, that it might be worth it to have separate tests |
Thanks guys for working on this. const toThenable = (fn) => (input) => {
try {
const result = fn(input)
if (result instanceof Promise) {
return result
}
return {
then(onFulfilled) { return toThenable(onFulfilled)(result) },
catch(onRejected) { return this },
}
} catch (e) {
return {
then(onFulfilled) { return this },
catch(onRejected) { return toThenable(onRejected)(e) },
}
}
} |
thanks @dai-shi I am going to playaround with your proposal |
@barbogast are agree to finalize the api first and then write tests ? |
Right now the code doesn't behave correctly if the Is this case supposed to be handled correctly? Thinking about it, a --- edit |
Yeah, as you said a single console.warn would be enough. |
Co-authored-by: Anatole Lucet <anatole@hey.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great, and getting close. There're a few things to fix for edge cases.
So sorry about the messy comments. If ambiguous, feel free to ask.
src/middleware.ts
Outdated
`Persist middleware: unable to update ${name}, the given storage is currently unavailable.` | ||
) | ||
} | ||
}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
}) | |
}).catch((e) => { throw e }) |
I'm pretty sure we want this, no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure if it's working in a sync
manner. let me check that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this magically works.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I meant it would work with my fix, which was wrong.
473c16a should be the fix.
Correctly handle version discrepancies with missing migrate functions
Here I have updated the gist. there are still typescript issues in |
Here you go: playground |
} | ||
}) | ||
.then((deserializedStorageValue) => { | ||
if (deserializedStorageValue) { | ||
if (deserializedStorageValue.version !== version) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (deserializedStorageValue.version !== version) { | |
if ((deserializedStorageValue.version ?? false) && deserializedStorageValue.version !== version) { |
see #409
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removing ?? false
just works?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not if the version is 0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, my bad. This doesn't even work if the version is 0
. So maybe something like: typeof deserializedStorageValue.version === "number" && deserializedStorageValue.version !== version
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
StorageValue<S>
requires version to be number
so we can even throw an error if typeof ... !== "number".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, that wouldn't permit to store a primitive value as in #409
But maybe there is a reason not to allow that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
well, if we want to allow non-version store, we should also change the StorageValue<S>
type. I'm fine with either way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we tackle it as a separate PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, let's merge this first
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great to me. Nice work!
Good job everybody. |
Like-wise, after years working with open-source software I'm still amazed getting such high-quality software for free. And in addition to that it feels like there's 24/7 support with direct access to the core-developers, no premium-support-subscription needed. Hehe, got all philosophical here 🙈 Anyway, thanks all 😍 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lgtm. Many thanks to everyone for your hard work! 🚀
WIP
middleware.ts
is completely migrated from Async/Await to Promise.and also sync apis will be called in a sync manner.
fixing #346