refactor(unmountAtom): use atomStateMap.get instead of ensureAtomState #3010
Conversation
When unmounting an atom, we don't need to ensure or create atom state since we only care about already mounted atoms. This change: - Prevents unnecessary atom state creation - Makes the unmounting logic clearer - Returns early if atom state doesn't exist BREAKING CHANGE: None
|
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. |
| @@ -838,7 +838,11 @@ const buildStore = ( | |||
| const unmountAtom = | |||
| buildingBlockFunctions[8] || | |||
| ((atom) => { | |||
| const atomState = ensureAtomState(atom) | |||
There was a problem hiding this comment.
I think we want to keep this, as this is the single point of accessing the atomStateMap. There's possibility to extend it, and I don't feel like adding more lines for this.
There was a problem hiding this comment.
@dai-shi
Got it! I thought it could be intentional too.
Maybe it's a small overhead, but I thought it was causing unnecessary work right now, and as a beginner, I couldn't determine if atomOnInit hooks might cause any unintended problems or not.
That seems pretty reasonable if it's just for the purpose of ensuring and adhering to the converntions.
Thank you for your reply!
There was a problem hiding this comment.
Yeah, for the normal use case, atom state should have been created in advance.
Related Bug Reports or Discussions
Fixes #
Summary
When unmounting an atom, we don't need to create atom state since we only care about already mounted atoms.
but ensureAtomState make new atom whenever there isn't atom.
I thought this process is kinda unnecessary
Check List
pnpm run fixfor formatting and linting code and docs