-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
docs(README): remove 'omit' in 'Overwriting state' section #3160
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
docs(README): remove 'omit' in 'Overwriting state' section #3160
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
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. |
6cf85c2
to
ef95383
Compare
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 we were to change the README, it would be nicer to avoid the library at all. Can you try?
README.md
Outdated
@@ -125,7 +125,7 @@ const treats = useBearStore( | |||
The `set` function has a second argument, `false` by default. Instead of merging, it will replace the state model. Be careful not to wipe out parts you rely on, like actions. | |||
|
|||
```jsx | |||
import { omit } from 'es-toolkit/compat' | |||
import omit from 'es-toolkit/compat/omit' |
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.
How does it look like?
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. I mean to avoid using the external library at 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.
I'm down with you @dai-shi, we should remove the external lib in this case and replace it with Object.fromEntries(...)
+ Object.entries()
+ .filter()
README.md
Outdated
import omit from 'lodash-es/omit' | ||
function omit(obj, keys) { | ||
const result = { ...obj } | ||
|
||
for (let i = 0; i < keys.length; i++) { | ||
const key = keys[i] | ||
delete result[key] | ||
} | ||
|
||
return result | ||
} |
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.
How does it look like?
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.
It looks like not FP, perhaps it should be like this:
const omit = keysToOmit => obj =>
Object.fromEntries(
Object.entries(obj).filter(([key]) => !keysToOmit.includes(key))
);
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.
function omit(obj, keys) {
return Object.fromEntries(
Object.entries(obj).filter(([key]) => !keys.includes(key))
)
}
I understand what you’re saying, but I’ve already taken browser compatibility into consideration for now.
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.
Please don't create a util function. Just do it in the store action in a simple way.
05d7c65
to
8fb8fbe
Compare
README.md
Outdated
import omit from 'lodash-es/omit' | ||
|
||
const useFishStore = create((set) => ({ | ||
salmon: 1, | ||
tuna: 2, | ||
deleteEverything: () => set({}, true), // clears the entire store, actions included | ||
deleteTuna: () => set((state) => omit(state, ['tuna']), true), | ||
deleteTuna: () => | ||
set((state) => { | ||
const { tuna, ...rest } = state | ||
return rest | ||
}, true), | ||
})) |
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 reflected your opinion.
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.
Cool. I'm not sure if we prefer this, but we could also do this.
set(({ tuna, ...rest }) => rest, true);
@dbritto-dev thoughts?
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.
Sounds good. I reflected your opinion.
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.
@dai-shi I'm down with you
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.
Thanks!!
Related Bug Reports or Discussions
Fixes #
Summary
Check List
pnpm run fix
for formatting and linting code and docs