fix: compatibility with Secure EcmaScript

[Jack-Works]

I fixed it by replacing Draft{Map, Set}.prototype.method = to Object.defineProperty(...)

[netlify]

✔️ Deploy Preview for quizzical-lovelace-dcbd6a canceled.

🔨 Explore the source changes: f4a3964

🔍 Inspect the deploy log: https://app.netlify.com/sites/quizzical-lovelace-dcbd6a/deploys/622431e9317aa40007288174

[Jack-Works]

using https://github.com/immerjs/immer/pull/914/files?diff=unified&w=1 hide white space will make this PR easier to review.

[mweststrate]

thanks! looks ok in general, but could you elaborate a bit more what problem with secure EcmaScript this solves? (Or what that is). Also it might be a bit cleaner to convert it to single Object.defineProperties calls?

[coveralls]

Pull Request Test Coverage Report for Build 1920713780

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

• For more information on this, see Tracking coverage changes with pull request builds.
• To avoid this issue with future PRs, see these Recommended CI Configurations.
• For a quick fix, rebase this PR at GitHub. Your next report should be accurate.

Details

• 121 of 121 (100.0%) changed or added relevant lines in 2 files are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage remained the same at 98.553%

---

Totals
Change from base Build 1774254151:	0.0%
Covered Lines:	786
Relevant Lines:	794

---

💛 - Coveralls

[Jack-Works]

Thanks for the reply. I have written a description about why this fix can resolve the problem, see MikeMcl/decimal.js-light#20 (comment)

I will do the changes you requested tomorrow.

[Jack-Works]

@mweststrate hi! can you review this again? thanks!

[mweststrate]

@Jack-Works I checked the proposal, but it is still in stage 1 and looks largely inactive for the last 2 years, so I don't see a reason to provide all other users with a slower and slightly less straightforward implementation atm? Typically only at stage 3 it becomes interesting for popular libs like immer to get involved.

[Jack-Works]

I don't think defineProperty will be slow... I think the Proxy is more serve than defineProperty. If immer can merge this, we can get rid of patch-package to modify node_modules...

[mweststrate]

patch-package is the perfect solution for cases like this. one-off, innocent looking changes for a single user has too many times come back to ruin the day later somewhere down the road for me in the past. Landing this PR would basically mean committing to not change any built-in objects with a prototype in the future, which is quite a blanket cheque. So saving the inconvenience of patch-package in an exotic setup is not a strong enough argument for a package that is used this widely imho.

[Jack-Works]

strange... it's working in node --frozen-intrinsics, so it should also works in SES lockdown()

[erights]

Please look again and consider reopening. The code in the proposed changes is simply better code than the code it replaces. The original code already uses a defineProperty for the size property as it must, since this is an accessor property. But even this points out why defineProperty/defineProperties is the right level of abstraction for overriding inherited properties. Property assignment is not a workable way to express property override --- both because the inherited property may be an accessor (like size) or because the inherited property may not be writable (for example, if the prototype has been frozen). For all these reasons, the class abstraction mechanism, introduced in EcmaScript 6, realizes the semantics of overriding inherited properties with defineProperty/defineProperties semantics rather than assign semantics.

The situation of frozen prototypes is not obscure, and changing code to live better with them is not an obscure improvement. This affects

• Hardened JavaScript, aka SES
• Node --frozen-intrinsics as noted above
• Compat with TC53 standard for JavaScript for embedded
• Moddable XS engine, which implements the TC53 standard.

Landing this PR would basically mean committing to not change any built-in objects with a prototype in the future, which is quite a blanket cheque.

I do not understand this argument. By accepting this PR, you'd only be removing an incompatibility with frozen prototypes. This is no more a commitment to frozen prototypes than is any other code that is already compatible with frozen prototypes.

[erights]

strange... it's working in node --frozen-intrinsics, so it should also works in SES lockdown()

@Jack-Works yes, that does seem strange! Do you understand why it does seem to work with node --frozen-intrinsics without your patch, but does not seem to work in SES / Hardened JS?

[Jack-Works]

@erights immer v10 is now compatible with ses.

[erights]

OMG thanks!

[erights]

Any clarification of the previous mysteries? Just curious.

[Jack-Works]

Any clarification of the previous mysteries? Just curious.

no idea, haven't inspected that since then

[mweststrate]

Probably because we're using classes now rather than constructing prototypes, due to targetting more modern JS versions

…

On Wed, 19 Apr 2023, 11:50 Jack Works, ***@***.***> wrote: Any clarification of the previous mysteries? Just curious. no idea, haven't inspected that since then — Reply to this email directly, view it on GitHub <#914 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAN4NBBNS2GJRGGQIAKKVT3XB6YM5ANCNFSM5PWN2SMQ> . You are receiving this because you were mentioned.Message ID: ***@***.***>