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

Immer documentation #232

Merged
merged 3 commits into from
Apr 25, 2020
Merged

Immer documentation #232

merged 3 commits into from
Apr 25, 2020

Conversation

labkey-nicka
Copy link
Contributor

Rationale

This adds rationale and usage documentation around for Immer. I researched getting this incorporated into our TypeDoc's generated docs but there is currently not supported.

Changes

  • Adds a documentation file about Immer in packages/components/docs.

Note I'd recommend reviewing this as it looks in GitHub: https://github.com/LabKey/labkey-ui-components/blob/fb_immer_docs/packages/components/docs/immer.md.

- outline why switching to Immer
- links to good resources for Immer
- provides scenarios highlighting aspects of note
Copy link
Contributor

@labkey-susanh labkey-susanh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great, Nick. I've made some suggestions, none of which are necessary.

My larger suggestion, to make this document most relevant going forward, would be to move the Rationale for Switching section to the bottom. That is most relevant now, of course, but once read, that content will be in the way of the use of this as a quick immer reference. But that's also a change that could be made later, so also not necessary.

packages/components/docs/immer.md Outdated Show resolved Hide resolved
packages/components/docs/immer.md Outdated Show resolved Hide resolved
packages/components/docs/immer.md Outdated Show resolved Hide resolved
packages/components/docs/immer.md Outdated Show resolved Hide resolved
packages/components/docs/immer.md Outdated Show resolved Hide resolved
packages/components/docs/immer.md Outdated Show resolved Hide resolved
packages/components/docs/immer.md Outdated Show resolved Hide resolved
packages/components/docs/immer.md Outdated Show resolved Hide resolved
packages/components/docs/immer.md Outdated Show resolved Hide resolved
packages/components/docs/immer.md Outdated Show resolved Hide resolved
Copy link
Contributor

@labkey-martyp labkey-martyp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall looks great, just a couple questions/comments

packages/components/docs/immer.md Outdated Show resolved Hide resolved
packages/components/docs/immer.md Outdated Show resolved Hide resolved

// Optionally, freeze the instance. Still works with produce but doesn't require it to have
// an immutable instance via construction.
Object.freeze(this);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So just out of curiosity and possibly for clarification in the docs, do you even need readonly properties if you're going to do Object.freeze in constructor? If not, could you then just do Object.freeze in the constructor and not have to use Draft<> wrapper in produce?

Copy link
Contributor

@labkey-martyp labkey-martyp Apr 23, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh I see Object.freeze is a runtime thing. Which I see is necessary because it is surprisingly easy to get around readonly with a slightly indirect reference to it. Maybe we should call out that this example is the template we should be using.

// readonly fail without Object.freeze(this)
let circle = new Circle(5);
let r = "radius";
circle[r] = 7;

console.log(circle.radius); // 7

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

readonly

Using readonly is intended to help prevent illicit behavior but, as you noted, the TS compiler can't catch everything. Minor thing, but if you changed it to const r = "radius" then TS does pick up the type and will throw an error.

Object.freeze

Unfortunately, Object.freeze also doesn't suffice in more complex cases because it is not a "deep freeze" Example:

'use strict';
let x = Object.freeze({ y: { z: 10 } });
x.y = {};            // TypeError: Cannot assign to read only property 'y' of object '#<Object>'
x.y.z = 20;          // OK! Because 'y' is not frozen.
console.log(x.y.z);  // 20

There isn't really a clean way (that I can think of) to have a deeply immutable class instance using Immer via the class' constructor only. It's kind of the inverse of the problem we have with ImmutableJS where ImmutableJS the instance is already immutable after the required call to super(). Ideally, I'd like to have new Circle(5) return me a deep-freezed immutable instance.

@labkey-nicka labkey-nicka merged commit 6ef2aae into master Apr 25, 2020
@labkey-nicka labkey-nicka deleted the fb_immer_docs branch April 25, 2020 13:52
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.

4 participants