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

Typescript initialization without change #381

Closed
dexx086 opened this issue Jun 6, 2019 · 13 comments
Closed

Typescript initialization without change #381

dexx086 opened this issue Jun 6, 2019 · 13 comments

Comments

@dexx086
Copy link

dexx086 commented Jun 6, 2019

First of all, thanks for this great library, I already love immer :)
But one thing bothers me, I hope someone can help.

What I'm trying to do: initialize an object as immutable but without any real property changes. Not the object instance is important for me (so the fact that it's not 'freezed' doesn't matter, because I know, in production it's already turned off by default). The only thing is important for me: to make Typescript catch any mistaken direct changes to the properties after I 'made it immutable'.

But the strange thing is that: if I give an empty producer in calling 'produce' (CASE 1), after that Typescript allows to directly modify property of the returned 'immutable' object (what I don't want, and don't understand why)!
But...if I do any simple modification in the producer (CASE 2), Typescript will not allow direct changes on the properties of the returned object (as expected).

Why (and how) Typescript differentiates between the 'empty' and the 'really modifying' producer? How could I initialize the immutable object without any real changes but making Typescript after that to catch when trying to modify readonly properties?

Thanks in advance.

import produce, { immerable } from "immer";

class TheClass {
  [immerable] = true;
  public title: string = "";
}

// Just initialize the object
let data = new TheClass();
// Want to make it IMMUTABLE from this point, to avoid any direct property changes

// CASE 1
let immutable1 = produce(data, (draft) => { /* empty, dont want to change anything */ });
// Typescript should NOT allow this, it should be immutable with readonly properties
immutable1.title = "shouldn't be able to modify..."; // But, it works....

// CASE 2
let immutable2 = produce(data, (draft) => { draft.title = "initial modification"; });
// Error (as expected): Cannot assign to read only property...
immutable2.title = "it won't work, Typescript catches it";

codesandbox

@mweststrate
Copy link
Collaborator

mweststrate commented Jun 6, 2019 via email

@dexx086
Copy link
Author

dexx086 commented Jun 6, 2019

I don't want to make the properties readonly, I just want to 'initialize' the object (without actually changing it) to be an immer's immutable object!

But for some reason, if I don't really modify a property inside the producer, the returned object is not handled as immutable in Typescript.
However, if I do an actual modification (to 'title' property), it WILL act as immutable object and Typescript disallows direct change to the properties on the returned object, and I can (as expected and wanted!) modify it only via the 'produce' mechanism.

Do you see why the two producers (empty / actual change) causes different types for the returned objects in Typescript? Because it's my only problem... CASE 2 is what I'd like to achieve but without the modification...

@dexx086
Copy link
Author

dexx086 commented Jun 7, 2019

Well, it turned out that the error message was not a compile error... the IDE on codesandbox highlighted it and it seemed like this was a type checking error at compile time, but actually it was a runtime error. The IDE just (btw cleverly but a bit confusingly) highlighted the errorneous line (even from runtime error)...
It would turned out earlier if I had checked this issue locally and would see that the compiler wouldn't complain about any readonly issue. Sorry for overcoming the error type.

So, it has nothing to do with Typescript, it's not a compile error, so it's not type-checkable if the class is really an immutable object, or still the original mutable one. I guess need to make sure for such cases to have the initial object somehow modified in the producer to force making it really an immutable one...

@dexx086 dexx086 closed this as completed Jun 7, 2019
@aleclarson
Copy link
Member

If you really want your objects frozen at runtime, you should use deep-freeze.

Immer will only freeze objects that were changed by the producer.

@dexx086
Copy link
Author

dexx086 commented Jun 7, 2019

You're right, I could manually freeze it with other tools, but as using Immer, would be much practical to use that for this case as well (all I'm looking for is a 'force freeze' funcionality).

I think my point about this whole thing is not clear, let me describe a usecase:

  1. initialize a class (with only defaults). It's a mutable one and this is the point I'd like to make it immediately immutable with immer
  2. the variable is initiated, and somewhere in the code I forgot to call produce to modify the object's property, instead modifying a property directly => it's not catched at runtime not even in development mode, it mutates the original object

It will be stay hidden if unluckily this is the first modification, and happens only once. This is why I'm looking for a bulletproof solution to make it (force) to be converted to immutable, and everywhere I'm using the immutable one, (kind-of) immediately turns out if a mistaken direct change stays in the code, because for the frozen object it will raise a (as turned out) runtime error.

Btw: isn't it possible to have return types from produce to reflect readonly properties for the returned object? That would allow us to have typescript compile time errors if ever trying to modify a property directly for an object that's returned from produce (even if actually that's the original object because no modification happened!). Just a thought.

@aleclarson
Copy link
Member

all I'm looking for is a 'force freeze' funcionality

Here's a solution that freezes any objects added to a draft: #260 (comment)

I'd like to make it immediately immutable with immer

You can wrap produce like this (more or less):

import {produce} from 'immer'
import deepFreeze from 'deep-freeze'
export function produceFrozen(base, recipe) {
  if (!Object.isFrozen(base)) deepFreeze(base)
  const result = produce(base, recipe)
  if (!Object.isFrozen(result)) deepFreeze(result)
  return result
}

Note: You should probably only use that when process.env.NODE_ENV !== 'production'

Btw: isn't it possible to have return types from produce to reflect readonly properties for the returned object?

I tried going that direction, and many people didn't like how produce made their types deeply readonly without their permission. See #289

@dexx086
Copy link
Author

dexx086 commented Jun 7, 2019

Thanks for your detailed feedback.
Using deep-freeze could be at least a runtime solution, but what I'm really pursuing is: type-safe solution. That's why the whole Typescript related issue came up for me.

I checked the #289 thread you mentioned: yeah, I see it brought up some objections about the change. Although I agree with that it might be a big change and maybe could had waited for the next major version, but I disagree about that it should not be introduced at all. Typescript is all about type safety, if somewhere it raised up problems, that needs to be solved, but should accept the even more type- and usage safetyness that it would bring in. And imho it would not make Immer "opinnionated".

But, I don't get why you idea with the import { mutableProduce as produce } from 'immer' lost after you asked for feedback about it. I think it's a great idea to have both, so whoever wants strict/not-so-strict type safety for a project, can use whichever he/she wants. Furthermore, adding a new export (although not by renaming current produce to mutableProduce and forcing to alias it, instead introducing a new immutableProduce or so) could be done in the next minor version update as well, as that would be a new, optional feature to opt-in for, and should not really make incompatibilities.

Is your idea totally rejected, or do you see it viable to be introduced (in any way) in the near future?

Meanwhile, I came up with a (little hacky and regarding future changes of Immer, possibly fragile) workaround solution based on your modification: a new 'produce' method.
(unfortunately have to copy some types from Immer's definitions, because those are not exported, plus need to 'cheat' with @ts-ignore at some point to fake casting)

// Need a copy, it's not exported from Immer
declare class Nothing {
	// This lets us do `Exclude<T, Nothing>`
	private _: any;
}

// Need a copy, it's not exported from Immer
type FromNothing<T> = T extends Nothing ? undefined : T;

/** The inferred return type of `produce` with Immutable<Base> return type */
export type ProducedRO<Base, Return> = Return extends void
	? Immutable<Base>
	: Return extends Promise<infer Result>
		? Promise<Result extends void ? Base : FromNothing<Result>>
		: FromNothing<Return>;

function produceRO<Base, D = Draft<Base>, Return = void>(base: Base, recipe?: (draft: D) => Return, listener?: PatchListener): ProducedRO<Base, Return> {
	let result = produce(base, recipe ? recipe : (): any => { }, listener);
	// @ts-ignore
	return result;
}

let data = new TheClass();
let immutable1 = produceRO(data);
immutable1.title = "won't work..."; // Typescript compile error as expected: Cannot assign to 'title' because it is a read-only property.

@aleclarson
Copy link
Member

@dexx086 Good idea! See #385

@dexx086 dexx086 changed the title Typescript with initialization without change Typescript initialization without change Jun 8, 2019
@dexx086
Copy link
Author

dexx086 commented Jun 8, 2019

Wow, didn't expect such a superfast response on my idea, especially not together with a commit! :)
(btw. the produceRO method name was not well-thought from my side, it was just a quick name... in case you feel need for a better name, for eg. with some abbreviation rather related to immutable/type-safe keywords?)

Thank you for your support. I hope introducing this option could be suitable for everbody.

@aleclarson
Copy link
Member

Yeah, produceStrict might be a better name. I'm open to any suggestions.

PS: My PR isn't ready for primetime, since the SafeReturn type it adds results in an incorrect type.

@mweststrate
Copy link
Collaborator

mweststrate commented Jun 8, 2019 via email

@aleclarson
Copy link
Member

@mweststrate That is disgusting to me, but maybe...? 😆

@mweststrate
Copy link
Collaborator

5.3 will introduce an castImmutable utility, which can be combined with produce to produce an immutable type. That is not as straight forward as this proposal, but also keeps the api a bit smaller and more composable, so let;s see if that suffices first. See also updated docs in the linked commit.

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 a pull request may close this issue.

3 participants