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): add persist api #624

Merged
merged 3 commits into from
Nov 2, 2021
Merged

feat(middleware): add persist api #624

merged 3 commits into from
Nov 2, 2021

Conversation

AnatoleLucet
Copy link
Collaborator

Closes #267, #513

@AnatoleLucet AnatoleLucet requested a review from dai-shi November 1, 2021 14:40
@codesandbox-ci
Copy link

codesandbox-ci bot commented Nov 1, 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 dd698f4:

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 1, 2021

Size Change: +498 B (+5%) 🔍

Total Size: 11.1 kB

Filename Size Change
dist/esm/middleware.js 2.78 kB +288 B (+12%) ⚠️
dist/middleware.js 2.94 kB +210 B (+8%) 🔍
ℹ️ View Unchanged
Filename Size
dist/context.js 622 B
dist/esm/context.js 521 B
dist/esm/index.js 1.2 kB
dist/esm/shallow.js 272 B
dist/esm/vanilla.js 520 B
dist/index.js 1.32 kB
dist/shallow.js 318 B
dist/vanilla.js 630 B

compressed-size-action

src/middleware.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.

It looks overall good. Some minor things and typing issues need to be addressed.

src/middleware.ts Outdated Show resolved Hide resolved
src/middleware.ts Outdated Show resolved Hide resolved
src/middleware.ts Outdated Show resolved Hide resolved
src/middleware.ts Show resolved Hide resolved
src/middleware.ts Show resolved Hide resolved
tests/middlewareTypes.test.tsx Outdated Show resolved Hide resolved
@AnatoleLucet
Copy link
Collaborator Author

Within the tests, the immer middleware keep the api as StoreApi. Shall I change this middleware using CustomStoreApi?

immer((set, get) => ({

There's still another minor type issue within the middleware with the create function, but I'm unable to find how to solve it. Could you check it out?

@dai-shi
Copy link
Member

dai-shi commented Nov 2, 2021

Shall I change this middleware using CustomStoreApi?

Yeah, I can't think of other ways.
This may require users to update their immer middleware.

There's still another minor type issue within the middleware with the create function, but I'm unable to find how to solve it. Could you check it out?

Hm, I don't know either. Let's add type assertion.

@dai-shi dai-shi self-requested a review November 2, 2021 10:42
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.

The code looks good.

@dai-shi dai-shi changed the title feat(persist middleware): add api feat(middleware): add persist api Nov 2, 2021
@dai-shi dai-shi merged commit 8665dd6 into main Nov 2, 2021
@dai-shi dai-shi deleted the feat/persist-api branch November 2, 2021 11:46
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.

Persist middleware api
2 participants