feat: non-security mode for create-react-scripts compat. (KLUDGE)#642
Conversation
84249a6 to
05714aa
Compare
dckc
left a comment
There was a problem hiding this comment.
LGTM, modulo "seems to be" typo?
| - No changes yet | ||
| - We temporarily introduced a new `__unsafeKludgeForReact__` option whose | ||
| `'unsafe'` setting that unsafely allows React to work under SES in the browser | ||
| even before React is fixed. Once React is fixed, this option will disappear. |
There was a problem hiding this comment.
I shy away from future tense. I tend toward "We plan to remove this once React is fixed." And I would much prefer to have an open issue to represent this plan.
There was a problem hiding this comment.
typo fixed. Creating the issue next.
kriskowal
left a comment
There was a problem hiding this comment.
The substance is fine. Calling out React specifically is unnecessary and casts unfair judgement. React is working as designed and ascribing brokenness is subjective to use with SES. The problem this addresses also applies equally well to any framework that includes a shim for any of the intrinsics. That is probably all of them.
The name of the option should express the solution more than the problem. In prose, this is to enable the use of the harden and assert shims without creating a SES environment, an environment that Agoric frontends using CapTP marshaling depend upon.
I would agree that “don’t harden intrinsics” fails to communicate the scope of the lack of safety, or that this is not a sound foundation in the long term. But, perhaps __immediatelyHardenedSharedIntrinsics__: "unsafe" has enough qualifiers. Maybe __hardenedIntrinsics__: "unsafe" for the sake of typos.
I get your point about removing references to React other than as an example. I will do that. However, this only happens to work for react because react happens to do all its monkey patching in ways that don't conflict with the freezing of primordials caused by transitive hardening. In that sense, this really is react specific, in that anything else suffering the same problem only slightly differently may not be rescued by this kludge. Hence I want to keep "unsafeKludge" in the name of the option. |
I like having |
I kept the "unsafe". It is now |
We temporarily introduce a new
__unsafeKludgeForReact__option whose'unsafe'setting that unsafely allows React to work under SES in the browsereven before React is fixed. Once React is fixed, this option will disappear.
Enables https://github.com/Agoric/dapp-token-economy/issues/159 to be fixed by injecting SES on the page first, as shown at https://github.com/Agoric/dapp-token-economy/pull/155
@dckc @kriskowal This is now ready for review. @dckc Because you created this PR I cannot add you as an official reviewer but please look anyway.