-
Notifications
You must be signed in to change notification settings - Fork 184
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
Use smaller React pragmas to minimise the amount of data passed between iframes #304
Conversation
🦋 Changeset detectedLatest commit: 286e42c The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Also needs a changeset. |
src/utils/compileJsx.ts
Outdated
}) | ||
); | ||
|
||
export const validateCode = (code: string): true | Error => { |
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.
Is there a specific reason for returning this mix of types? The only usage of the error as I see it is up in the code editor, and this was refactored away from a try/catch.
I think this may just be a refactoring relic from compiling with sucrase.
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.
validateCode
calls compileJsxWithBabel
, not compileJsx
. I didn't want to expose compileJsxWithBabel
, so I merged the return types.
Here it needs the true
value:
Lines 32 to 38 in 2e889eb
: validateCode( | |
insertAtCursor({ | |
code, | |
cursor, | |
snippet: breakoutString, | |
}) | |
) === true; |
Here it needs the error:
playroom/src/Playroom/CodeEditor/CodeEditor.tsx
Lines 47 to 49 in 2e889eb
const validOrError = validateCode(code); | |
if (validOrError !== true) { | |
const errorMessage = validOrError.message; |
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.
Not a blocker, but perf has regressed a bit in large playrooms. Seems somewhere inbetween the responsiveness of the pre-sucrase version and current playroom master. Wonder if we could debounce the validation (in a separate PR)?
braid.playrooom-react-pragmas.URL.txt (from this braid branch)
That is to be expected, since we are running the Babel transform just for validation and Sucrase transform for the actual compilation. Ideally we would just parse, not transform. 8b91070...smaller-react-pragmas I'll run some tests with Braid. |
Tested the latest commit's snapshot in a large braid playroom, and it feels just as good as the sucrase-only version, maybe even slightly better. 👏 |
Fixes #297.
Happy to bikeshed the names
R_F
andR_cE
because they might clash with other global identifiers that were provided via thescope
option.The shortened pragma names should not contain any characters that need to be encoded, so only use characters from this list (while still being valid JavaScript identifier names):