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(middleware): devtools to patch api.setState with namedSet and fix middleware types #634

Merged
merged 6 commits into from
Nov 3, 2021

Conversation

dai-shi
Copy link
Member

@dai-shi dai-shi commented Nov 3, 2021

close #517

@codesandbox-ci
Copy link

codesandbox-ci bot commented Nov 3, 2021

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 39d8e66:

Sandbox Source
React Configuration
React Typescript Configuration
React Browserify Configuration
React Snowpack Configuration
React Parcel Configuration
Next.js Configuration

@github-actions
Copy link

github-actions bot commented Nov 3, 2021

Size Change: -18 B (0%)

Total Size: 11.2 kB

Filename Size Change
dist/esm/middleware.js 2.77 kB -8 B (0%)
dist/middleware.js 2.93 kB -10 B (0%)
ℹ️ View Unchanged
Filename Size
dist/context.js 622 B
dist/esm/context.js 521 B
dist/esm/index.js 1.23 kB
dist/esm/shallow.js 272 B
dist/esm/vanilla.js 520 B
dist/index.js 1.35 kB
dist/shallow.js 318 B
dist/vanilla.js 630 B

compressed-size-action

@dai-shi
Copy link
Member Author

dai-shi commented Nov 3, 2021

typescript playground

I guess I understand what was wrong.

@dai-shi dai-shi changed the title wip(middleware): devtools to patch api.setState with namedSet feat(middleware): devtools to patch api.setState with namedSet and fix middleware types Nov 3, 2021
@dai-shi dai-shi marked this pull request as ready for review November 3, 2021 14:47
@barelyhuman
Copy link
Collaborator

barelyhuman commented Nov 3, 2021

doesn't this end up adding the initial concern that we had about the function knowing something extra compared to others?

I mean,
it works but then simple optional type would've sufficed for the initial namedSet state from barelyhuman#11

@dai-shi
Copy link
Member Author

dai-shi commented Nov 3, 2021

Do you mean people can add custom store type without using its middleware?
Yes, that's correct. But, I guess it's what #617 did.

At least, if you use devtools and you forgot to annotate custom store type, you will get an error. So, I guess it's technically better.
And, if we use devtools & combine, we don't need to annotate types, which was really my goal for this.

Most importantly, vanilla.ts doesn't know anything about middleware.

@barelyhuman
Copy link
Collaborator

The last point makes the change a valid decision. Still a bit on the complicated side since we've got a lot on inner extends that might get hard to control or understand over time.
Otherwise, looks good.

@dai-shi
Copy link
Member Author

dai-shi commented Nov 3, 2021

Still a bit on the complicated side

I don't disagree, it's complicated.
From my perspective, when I did #601, I wasn't comfortable very much and felt something wrong. With some feedback and discussions, I revisited in #617, and it's much cleaner than before, and easier to educate users (it's verbose but straightforward). Now, I have a bit clearer understanding since then, and this PR improves some types. With this change, I feel this is a good balance between the library maintainability and good developer experience.

@dai-shi dai-shi merged commit 2bc4de3 into main Nov 3, 2021
@dai-shi dai-shi deleted the feat/devtools/patch-api-setstate branch November 3, 2021 22:30
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.

[Question] Name actions differently in devtools with setState api returned from create
3 participants