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(utils): add proxy set #325

Merged
merged 16 commits into from
Jan 17, 2022
Merged

feat(utils): add proxy set #325

merged 16 commits into from
Jan 17, 2022

Conversation

fkhadra
Copy link
Collaborator

@fkhadra fkhadra commented Jan 13, 2022

I created a new PR as I rebased from fix(core): expose canProxy in handler #321. Below is the output of the tests that are generated. I can of course add more if needed.
Screenshot 2022-01-13 at 15 20 48

I was thinking to add an example as well inside the example folder todo-with-proxySet or whatever the name. Let me know if you think it's worth it

dai-shi and others added 10 commits January 11, 2022 22:53
- `initialValues` signature match native set
- use `Array.from` during instantiation
- wrap object in proxy when calling `has`
- `toJSON` match native set behavior
The tests use a table testing approach. The idea is to run the same tests with
differents input
@vercel
Copy link

vercel bot commented Jan 13, 2022

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/pmndrs/valtio/2PWcViXEBHPsgp67XunpWC6ZTbua
✅ Preview: https://valtio-git-fork-fkhadra-feat-add-proxy-set-pmndrs.vercel.app

@codesandbox-ci
Copy link

codesandbox-ci bot commented Jan 13, 2022

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 353f0cf:

Sandbox Source
React Configuration
React Typescript Configuration
React Browserify Configuration
React Snowpack Configuration
React Parcel Configuration

@dai-shi
Copy link
Member

dai-shi commented Jan 13, 2022

I was thinking to add an example as well inside the example folder todo-with-proxySet or whatever the name. Let me know if you think it's worth it

That's a very nice idea!

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.

Once again, this is cool. Look forward to merging.

#321 is merged. Please merge (or rebase from) the main branch.

src/utils/proxySet.ts Outdated Show resolved Hide resolved
src/utils/proxySet.ts Outdated Show resolved Hide resolved
src/utils/proxySet.ts Outdated Show resolved Hide resolved
src/utils/proxySet.ts Outdated Show resolved Hide resolved
src/utils/proxySet.ts Outdated Show resolved Hide resolved
src/utils/proxySet.ts Outdated Show resolved Hide resolved
src/utils/proxySet.ts Outdated Show resolved Hide resolved
src/utils/proxySet.ts Show resolved Hide resolved
src/utils/proxySet.ts Show resolved Hide resolved
src/utils/proxySet.ts Show resolved Hide resolved
src/utils/proxySet.ts Outdated Show resolved Hide resolved
- use type instead of interface
- update initialValues signature
- fix object check
- use splice instead of reassignment to avoid creating a new proxy
- add tests for proxySet internal
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.

Getting better and better.

src/utils/proxySet.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.

LGTM
This is very good. It doesn't look so magical. (Not easy for everyone though, like how this works for snapshots.)
Thanks for your work!

@fkhadra
Copy link
Collaborator Author

fkhadra commented Jan 15, 2022

Amazing, do you want me to add an example before we merge? I'm trying to figure out a good use case(a simple one) for using Set. I thought first about the todo but most of the operations relies on the todo id which makes sense

@dai-shi
Copy link
Member

dai-shi commented Jan 15, 2022

You can work on examples in a separate PR. Can you add a short section in readme?

@fkhadra
Copy link
Collaborator Author

fkhadra commented Jan 15, 2022

You can work on examples in a separate PR. Can you add a short section in readme?

Sure, no problem I'll do this later today when I'm back. I've also pushed another commit to remove the writable from data as we don't reassign it.

@fkhadra
Copy link
Collaborator Author

fkhadra commented Jan 17, 2022

Added a tiny readme section let me know if it's enough

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.

sure, let's merge this and ship it.

@dai-shi dai-shi merged commit 660f2ed into pmndrs:main Jan 17, 2022
@dai-shi dai-shi changed the title Feat/add proxy set feat(utils): add proxy set Jan 17, 2022
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.

2 participants