Typescript 3.9 breaks getters in extendObservable

[JabX]

Typescript 3.9 introduced a sneaky breaking change regarding the way getter are emitted by setting enumerable: false instead of enumerable: true (associated PR here).

This causes extendObservable to ignore properties defined as getters in its second parameter, like that :

const obs = extendObservable({a: 2, b: 3}, { get c() { return a + b; });
obs.c // undefined (!!)

I'm a bit surprised that no one has yet reported this, it literally broke my entire app once I upgraded.

I'm not sure how this can be mitigated : how can extendObservable enumerate the properties to add if they aren't actually enumerable?

[danielkcz]

Interesting observation. It doesn't seem we can really do much here. I see some complaint in the linked PR so hopefully, they will give it a second thought. I guess you have to stick to the previous TS till then.

[mweststrate]

Non enumerable props can be iterated through Object.getOwnPropertyNames w/o problem. I think in MobX 6 this would actually work already. MobX 4/5 might need a patch if this is default behaviour in TS

…

On Sun, 28 Jun 2020, 22:33 Daniel K., ***@***.***> wrote: Interesting observation. It doesn't seem we can really do much here. I see some complaint in the linked PR so hopefully, that will give it a second thought. I guess you have to stick to the previous TS till then. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#2386 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAN4NBGUHDQCYHPFUR2VELLRY6ZL7ANCNFSM4OKWLMRQ> .

[JabX]

This is actually the only behavior that TS supports now, there is no plan to add a flag or something to control it. I ended up reverting to 3.8 for the time being.

They keep mentioning classes though in their justification (and in the existing complaints), but here I'm not using a class at all, just a plain JS object. Is the behavior really supposed to be the same here?

I forgot to mention, but I am using MobX 4 (latest), so I don't know if it also happens in 5.

If we were to switch to using Object.getOwnPropertyNames, isn't it going to break something else? Like weird inheritance-based patterns or something?

[mweststrate]

I think there would be little risk in updating that, as the second argument of extendObservable is supposed to be a plain object anyway.

[mweststrate]

Anybody interested in making a PR for this one on v5/4? I think the unit tests from 6 could be reused directly:

mobx/test/v5/base/make-observable.ts

Lines 338 to 357 in 7c5ed6c

	test("extendObservable can be used late and support non-enumerable getters #2386", () => {
	function MyClass() {
	const args = {
	x: 1,
	inc() {
	this.x++
	}
	}
	Object.defineProperty(args, "double", {
	get() {
	return this.x
	},
	enumerable: false
	})
	extendObservable(this, args)
	}
	const i = new MyClass()
	expect(isObservableProp(i, "x")).toBe(true)
	expect(isComputedProp(i, "double")).toBe(true)

[mweststrate]

Fixed and released as (4/5).14.5