Skip to content
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

Merged
merged 11 commits into from Nov 23, 2023

Conversation

mrm007
Copy link
Contributor

@mrm007 mrm007 commented Nov 17, 2023

Fixes #297.

Happy to bikeshed the names R_F and R_cE because they might clash with other global identifiers that were provided via the scope 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):

A–Z a–z 0–9 - _ . ! ~ * ' ( )

@mrm007 mrm007 requested a review from a team as a code owner November 17, 2023 00:39
Copy link

changeset-bot bot commented Nov 17, 2023

🦋 Changeset detected

Latest commit: 286e42c

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
playroom Minor

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

@askoufis
Copy link
Contributor

Also needs a changeset.

@mrm007 mrm007 marked this pull request as draft November 17, 2023 01:22
@mrm007 mrm007 marked this pull request as ready for review November 21, 2023 00:57
})
);

export const validateCode = (code: string): true | Error => {
Copy link
Contributor

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.

Copy link
Contributor Author

@mrm007 mrm007 Nov 21, 2023

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.

call hierarchy

Here it needs the true value:

: validateCode(
insertAtCursor({
code,
cursor,
snippet: breakoutString,
})
) === true;

Here it needs the error:

const validOrError = validateCode(code);
if (validOrError !== true) {
const errorMessage = validOrError.message;

Copy link
Contributor

@askoufis askoufis left a 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)

@mrm007
Copy link
Contributor Author

mrm007 commented Nov 21, 2023

Seems somewhere inbetween the responsiveness of the pre-sucrase version and current playroom master.

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.

src/utils/compileJsx.ts Outdated Show resolved Hide resolved
@askoufis
Copy link
Contributor

askoufis commented Nov 22, 2023

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. 👏

braid.playrooom-react-pragmas-NEW.URL.txt

@mrm007 mrm007 merged commit 1c8ae6b into master Nov 23, 2023
7 checks passed
@mrm007 mrm007 deleted the smaller-react-pragmas branch November 23, 2023 00:06
@mrm007 mrm007 mentioned this pull request Nov 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Code given to frames aren't compressed
3 participants