-
Notifications
You must be signed in to change notification settings - Fork 74
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
feat: non-security mode for create-react-scripts compat. (KLUDGE) #642
Conversation
84249a6
to
05714aa
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, modulo "seems to be" typo?
packages/ses/NEWS.md
Outdated
- 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo fixed. Creating the issue next.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
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.