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: freeze atom util #185

Merged
merged 8 commits into from
Nov 25, 2020
Merged

feat: freeze atom util #185

merged 8 commits into from
Nov 25, 2020

Conversation

dai-shi
Copy link
Member

@dai-shi dai-shi commented Nov 10, 2020

ref: #167

Usage 1

import { atom } from 'jotai'
import { freezeAtom } from 'jotai/utils'

const countAtom = freezeAtom(atom(0))

Usage 2

import { atomFrozenInDev as atom } from 'jotai/utilis'

const countAtom = atom(0)
// this is already frozen (DEV only)

@codesandbox-ci
Copy link

codesandbox-ci bot commented Nov 10, 2020

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 6804270:

Sandbox Source
React Configuration
React Typescript Configuration

@likern
Copy link
Contributor

likern commented Nov 10, 2020

ref: #167

Usage 1

import { atom } from 'jotai'
import { freezeAtom } from 'jotai/utils'

const countAtom = atom(0)
freezeAtom(countAtom)

I think there might be expectation that freezeAtom returns freezed object, while original is allowed to be modified as is (not freezed). But Object.freeze() is a side-effect function and countAtom will be freezed too.

If this is intentional I think it's better to not return anything from freezeAtom and make it pure side-effect.

@dai-shi
Copy link
Member Author

dai-shi commented Nov 10, 2020

It’s intentional and I can accept your suggestion.

@mdingena
Copy link

mdingena commented Nov 11, 2020

I'd suggest to encourage functional programming patterns, and make freezeAtom a pure function that has no side-effects. This involves first cloning the object-to-freeze before freezing and returning it. It's a bit more work 🤷

@dai-shi
Copy link
Member Author

dai-shi commented Nov 11, 2020

@mdingena

make freezeAtom a pure function that has no side-effects.

Do you mean like this?

export function freezeAtom<T extends Atom<unknown>>(anAtom: T) {
  const origRead = anAtom.read
  return {
    ...anAtom,
    read: (get: Getter) => {
      const value = origRead(get)
      deepFreeze(value)
      return value
    }
  }
}

I'm fine with this too. Just thought originally that people expect freeze to have side effects, analogous to Object.freeze.

Another option is we don't export freezeAtom, just atomFroznInDev. How does that sound?

cc @likern

@likern
Copy link
Contributor

likern commented Nov 11, 2020

I'd suggest to encourage functional programming patterns, and make freezeAtom a pure function that has no side-effects. This involves first cloning the object-to-freeze before freezing and returning it. It's a bit more work

It will require deep-cloning, otherwise original object will be affected. I think performance penalty will be significant. If we will deep-clone (correct, if I am wrong) no need to freeze object.

I think to make it pure side-effect is better. Providing freezeAtom might be helpful too.

@dai-shi
Copy link
Member Author

dai-shi commented Nov 11, 2020

Here's a clarification just in case.

Let's not confuse atom configs and atom values.
Atom configs are object that can be created with atom() function.
Atom values are something that we receive/update with useAtom() hook, or in get/set in derived atoms.

Now, freezeAtom is a util function to create/modify an atom config that will deep freeze its atom value(s). It doesn't freeze atom config object, so it can be a pure function.

Object.freeze and deepFreeze are functions with side effects.

@likern
Copy link
Contributor

likern commented Nov 12, 2020

Yes, I understand that. freezeAtom is doing something with value itself (at time when read will be called). If we "freeze atom" we expect value associated with this atom will be frozen.

But when we will use original atom there is no such expectation, especially when freezeAtomlooks like pure-function, but doing side effect.

I would expect this code should work

const frozenAtom = freezeAtom(nonFrozenAtom)
...
const value = useAtom(nonFrozenAtom)
value.property = 'changed'

but because of freezeAtom side-effect it might not.

@dai-shi
Copy link
Member Author

dai-shi commented Nov 12, 2020

Yes, I understand that.

OK, was just in case.

I would expect this code should work

The code snippet I wrote in #185 (comment) makes it possible, no?

Hm, that code wasn't very nice. It's not updating key.
If we need pure freezeAtom, it should be the one in #167 (comment).

Let me update this PR and see how it goes.

@dai-shi
Copy link
Member Author

dai-shi commented Nov 12, 2020

Updated the description.
Note: freezeAtom now creates a derived atom.

@mdingena
Copy link

Maybe a dumb suggestion but what if we had:

import { atom } from 'jotai';

const myAtom = atom(42);

myAtom.freeze();

There is absolutely no doubt that this method has the side effect of changing myAtom.

@dai-shi
Copy link
Member Author

dai-shi commented Nov 12, 2020

@mdingena Thanks for ideation and suggestion. I agree that's a cleaner api. I enjoy these conversations.

I'd suggest to encourage functional programming patterns

However, I'd prefer this fp way.

myAtom.freeze();

This requires adding a property and it may increase a production bundle a bit, which I prefer to avoid.


We had some discussions in #167, but currently what I think is:

  • We want to keep jotai core minimal.
  • I'm a bit hesitant to add deepEqual in core.
  • Now, I look for a possibility to provide not ideal but reasonable api in utils.

@dai-shi dai-shi linked an issue Nov 15, 2020 that may be closed by this pull request
@dai-shi
Copy link
Member Author

dai-shi commented Nov 15, 2020

Should we move forward with this, or still gather more ideas?

@dai-shi
Copy link
Member Author

dai-shi commented Nov 17, 2020

I think it's good to go as utils without affecting core.

@dai-shi dai-shi marked this pull request as ready for review November 17, 2020 07:29
@dai-shi dai-shi merged commit 7601de9 into master Nov 25, 2020
@dai-shi dai-shi deleted the feat/freeze-atom branch November 25, 2020 22:28
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.

Discussion: atom values should be freezed/immutable
3 participants