Unexpected access to getter property in irrelevant plain objects

[zthxxx]

🙋‍♂ Question

Here is two question, when will a plain object contain drafts in the recursively finalize function?

I think a plain object here always means not modify in produce() and not create by immer proxy, maybe move a proxy draft property into new object then add it back to draft's new property field?

immer/src/core/finalize.ts

Lines 62 to 71 in 8e12c78

	// A plain object, might need freezing, might contain drafts
	if (!state) {
	each(
	value,
	(key, childValue) =>
	finalizeProperty(rootScope, state, value, key, childValue, path),
	true // See #590, don't recurse into non-enumerable of non drafted objects
	)
	return value
	}

Is that the only thing need to do in this condition for plain object is maybeFreeze(rootScope, value, true) rather than each(finalizeProperty(...))) ?

I think this is the reason to cause some UNEXPECTED access to getter property in an irrelevant plain object when produce() returns result, which will have some side effects.

The problem case like below:

Link to repro

To reproduce: https://stackblitz.com/edit/node-pgmcfz?file=immer-getter-without-freeze.mjs

Environment

• Immer version: 9.0.17
• Occurs with setUseProxies(true)
• Occurs with setUseProxies(false) (ES5 only)

[mweststrate]

Usually the quickest way to find these things out, is to disable that piece of code and see if a test breaks. Not sure if I understood your question correctly, but yes, I think it is indeed for "wrapping" cases like: draft.x = { y: draft.y }, which creates a plain object wrapping drafts. That is more common than you'd might expect since draft.todos = draft.todos.filter(x => x.done) does basically already create a plain object (well, array) wrapper around drafts.

[zthxxx]

@mweststrate This explanation is make scene to me, then I think ANOTHER reason in deep is the 93 line in each function,

it access obj[key] directly so that some getter will be called, but it's unnecessary in most of usage case call each function like "freeze property", on the other hand there are still a few cases need copy getter method or get value.

immer/src/utils/common.ts

Lines 90 to 98 in 8e12c78

	export function each(obj: any, iter: any, enumerableOnly = false) {
	if (getArchtype(obj) === Archtype.Object) {
	;(enumerableOnly ? Object.keys : ownKeys)(obj).forEach(key => {
	if (!enumerableOnly || typeof key !== "symbol") iter(key, obj[key], obj)
	})
	} else {
	obj.forEach((entry: any, index: any) => iter(index, entry, obj))
	}
	}

In any case, immer shouldn't even have to try freeze a getter, and for performance, while immer is copy-on-write, so I think the two cases in your explanation,

draft.x = { y: draft.y }
draft.todos = draft.todos.filter(x => x.done)

it shoud write draft.x and draft.todos to proxy draft object rather than plain object/array, means the proxy object of { y: draft.y } or array of draft.todos.filter(x => x.done) should created and in draft[Symbol.for("immer-state")].drafts_,

the process deal in "write" step of produce's recipe instead of in "return" step with ending of produce.

[mweststrate]

I'm not sure there is an easy non-expensive way of skipping over calling the getters, but feel to give it a try. In any case, having (non-idempotent) side effects usually creates problems in any case, even without Immer.

[zthxxx]

Actually, I have a series of array with each item that has a getter property to make lazy evaluations when they need to be used for the performance reasons, but immer called all of them even only make irrelevant modifications in produce(), so our page is stuck.

It's not really lacking side effects, but just have relatively more computationally intensive.

[mweststrate]

Yeah getters in an immutable tree are always a bit of a strange thing. Using a separate weakmap instead can be a cleaner solution. As an ugly workaround you could keep a global flag to prevent computations, or maybe `isDraft(this)` works as well in the getter. Feel free to submit a PR that would address this issue (upfront I don't give merge guarantees, as that will depend on correctness, perf impact, maintability and size)

…

On Wed, Jan 11, 2023 at 9:59 AM zthxxx ***@***.***> wrote: Actually, I have a series of array with each item that has a getter property to make lazy calculations when they need to be used for the performance reasons, but immer called all of them even only make irrelevant modifications in produce(), so our page is stuck. — Reply to this email directly, view it on GitHub <#1012 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAN4NBFY5ZGMN25JWL5AJNLWRZY6LANCNFSM6AAAAAATXFLJD4> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[mweststrate]

Immer 10 changed the approached to getters, so closing here.