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

feat: Support promise in watch util #825

Merged
merged 6 commits into from
Nov 28, 2023

Conversation

LiamMartens
Copy link
Contributor

Related Issues or Discussions

#507

Summary

Updates the watch util to support a promise callback. Proxies are subscribed to immediately to improve the time to first callback. This way a proxy can be subscribed to before the promise finishes fully.

Check List

  • yarn run prettier for formatting code and docs

Copy link

vercel bot commented Nov 18, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
valtio ✅ Ready (Inspect) Visit Preview 💬 Add feedback Nov 28, 2023 0:52am

Copy link

codesandbox-ci bot commented Nov 18, 2023

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 554fc73:

Sandbox Source
React Configuration
React TypeScript Configuration
React Browserify Configuration
React Snowpack Configuration
React Parcel Configuration

@dai-shi
Copy link
Member

dai-shi commented Nov 22, 2023

Thanks for working on it. Been busy, but hopefully, I'll be able to take a look sometime soon.

Copy link
Member

@dai-shi dai-shi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey, thanks for working on it and sorry for the delay.
I'm not super confident with the watch code, but if we have good test coverage, it will be mergeable.

Just a small request is to change the diff in this PR as minimal as possible.

src/vanilla.ts Outdated Show resolved Hide resolved
src/vanilla/utils/watch.ts Outdated Show resolved Hide resolved
src/vanilla/utils/watch.ts Show resolved Hide resolved
src/vanilla/utils/watch.ts Show resolved Hide resolved
src/vanilla/utils/watch.ts Show resolved Hide resolved
src/vanilla/utils/watch.ts Show resolved Hide resolved
src/vanilla/utils/watch.ts Outdated Show resolved Hide resolved
tests/watch.test.tsx Outdated Show resolved Hide resolved
tests/watch.test.tsx Outdated Show resolved Hide resolved
})

vi.runAllTimers()
await waitPromise
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this instance yes - because whilst the timers will be run - the code is still synchronous and won't wait for the other scheduled tasks to finish - like the promise itself which is scheduled as a macro task so it would resolve after the current function call. It's similar to why you do await Promise.resolve()

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, yeah. Right. My oversight.

src/vanilla/utils/watch.ts Outdated Show resolved Hide resolved
Copy link
Member

@dai-shi dai-shi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your contribution!

@dai-shi dai-shi merged commit 5950195 into pmndrs:main Nov 28, 2023
33 checks passed
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

Successfully merging this pull request may close these issues.

None yet

2 participants