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

[lexical-react] Bug Fix: Use automatic jsx runtime with react/jsx-runtime -> react alias in www #6143

Merged
merged 1 commit into from
May 23, 2024

Conversation

etrepum
Copy link
Collaborator

@etrepum etrepum commented May 20, 2024

Description

Changes www builds to use automatic jsx runtime with the react/jsx-runtime module aliased to react.

As far as I can tell react's index.fb.js is used as the module's entrypoint for fb builds and it includes the JSX runtime (Fragment is already present in the react module).

This should solve the problems that #6134 and #6140 are trying to work around in a more systematic and future-compatible way.

Test plan

Someone with access to intern will have to do a sync and make sure that this does indeed work.

Copy link

vercel bot commented May 20, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
lexical ✅ Ready (Inspect) Visit Preview 💬 Add feedback May 20, 2024 1:47pm
lexical-playground ✅ Ready (Inspect) Visit Preview 💬 Add feedback May 20, 2024 1:47pm

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label May 20, 2024
@etrepum
Copy link
Collaborator Author

etrepum commented May 20, 2024

/cc @Sahejkm

Copy link

size-limit report 📦

Path Size Loading time (3g) Running time (snapdragon) Total time
packages/lexical/dist/Lexical.js 23.91 KB (0%) 479 ms (0%) 230 ms (+23.67% 🔺) 708 ms
packages/lexical-rich-text/dist/LexicalRichText.js 34.6 KB (0%) 693 ms (0%) 450 ms (+3.55% 🔺) 1.2 s
packages/lexical-plain-text/dist/LexicalPlainText.js 34.51 KB (0%) 691 ms (0%) 382 ms (-22.77% 🔽) 1.1 s

Copy link
Contributor

@Sahejkm Sahejkm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@etrepum
Copy link
Collaborator Author

etrepum commented May 20, 2024

Note that there are no failing tests here, the approval labeling workflow still isn't working under some conditions #6136 #6120

@Sahejkm
Copy link
Contributor

Sahejkm commented May 21, 2024

Sorry about this, i am asking the OSS support for the next steps.

Looks like "pull_request_review" event was triggered , i missed while reviewing Nicolas added "pull_request_review" back, but this doesn't help with PRs from fork again :(

i have listed the problem as below:

PROBLEM WE ARE TRYING TO SOLVE
We are trying to add a workflow in Lexical to
Add Label to a PR if it doesn't have the label
Run e2e tests for the PR if label added
We want to add label as we don't want to have multiple runs of e2e tests as we are trying to solve the Repo Over budget issue

CURRENT APPROACH
we wrote this workflow file for the same:
https://github.com/.../actions/runs/9160647827/workflow
This workflow works well for PRs from Meta employees but it fails for PRs from Forks / external contributors with the error
GraphQL: Resource not accessible by integration (addLabelsToLabelable)
https://github.com/.../runs/9160647827/job/25183782894

NEXT STEPS TO FIX FAILURE IN CURRENT APPROACH
We need help on resolving this issue. For Github sources we learnt following things:

pull_request_target : this action gives the github workflow actions permissions for read/write from forked PR. But this action doesn't help us as we want to trigger this action on "Approval" , so only option we have is "pull_request_review" which doesn't have permissions to add Labels on PRs from forks
leverage PAT token for this workflow to add labels ?
settings to allow GithubWorkflow Actions Access to PRs from fork (followed this OSS WP group, looks like this option is disabled by default and is not recommended to use this for security reasons)
If this option is not possible other closest option we have is to run e2e tests on merge queue.

@Sahejkm Sahejkm added extended-tests Run extended e2e tests on a PR and removed extended-tests Run extended e2e tests on a PR labels May 21, 2024
@StyleT StyleT added this pull request to the merge queue May 23, 2024
Merged via the queue into facebook:main with commit 758b0ed May 23, 2024
40 of 42 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants