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

Re-enable prettier for compiler/ #29993

Merged
merged 2 commits into from
Jun 20, 2024
Merged

Re-enable prettier for compiler/ #29993

merged 2 commits into from
Jun 20, 2024

Conversation

poteto
Copy link
Member

@poteto poteto commented Jun 20, 2024

Stack from ghstack (oldest at bottom):

Now that the compiler directory has its own prettier config, we can
remove the prettierignore entry for compiler/ so it still runs in your
editor if you open the root directory

[ghstack-poisoned]
Copy link

vercel bot commented Jun 20, 2024

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

Name Status Preview Comments Updated (UTC)
react-compiler-playground ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jun 20, 2024 4:03pm

poteto added a commit that referenced this pull request Jun 20, 2024
Now that the compiler directory has its own prettier config, we can
remove the prettierignore entry for compiler/ so it still runs in your
editor if you open the root directory

ghstack-source-id: aeef8fedf3bac7c81a81d5bec5bc32100da65174
Pull Request resolved: #29993
@facebook-github-bot facebook-github-bot added CLA Signed React Core Team Opened by a member of the React Core Team labels Jun 20, 2024
@react-sizebot
Copy link

react-sizebot commented Jun 20, 2024

Comparing: b15c849...a78be41

Critical size changes

Includes critical production bundles, as well as any change greater than 2%:

Name +/- Base Current +/- gzip Base gzip Current gzip
oss-stable/react-dom/cjs/react-dom.production.js = 6.66 kB 6.66 kB +0.05% 1.82 kB 1.82 kB
oss-stable/react-dom/cjs/react-dom-client.production.js = 497.93 kB 497.93 kB = 89.26 kB 89.26 kB
oss-experimental/react-dom/cjs/react-dom.production.js = 6.67 kB 6.67 kB +0.05% 1.83 kB 1.83 kB
oss-experimental/react-dom/cjs/react-dom-client.production.js = 502.75 kB 502.75 kB = 89.96 kB 89.96 kB
facebook-www/ReactDOM-prod.classic.js = 597.17 kB 597.17 kB = 105.33 kB 105.33 kB
facebook-www/ReactDOM-prod.modern.js = 571.52 kB 571.52 kB = 101.27 kB 101.27 kB
test_utils/ReactAllWarnings.js Deleted 62.88 kB 0.00 kB Deleted 15.69 kB 0.00 kB

Significant size changes

Includes any change greater than 0.2%:

Expand to show
Name +/- Base Current +/- gzip Base gzip Current gzip
test_utils/ReactAllWarnings.js Deleted 62.88 kB 0.00 kB Deleted 15.69 kB 0.00 kB

Generated by 🚫 dangerJS against a78be41

[ghstack-poisoned]
poteto added a commit that referenced this pull request Jun 20, 2024
Now that the compiler directory has its own prettier config, we can
remove the prettierignore entry for compiler/ so it still runs in your
editor if you open the root directory

ghstack-source-id: 5e3bd597cf2f11a9931f084eb909ffd81ebdca81
Pull Request resolved: #29993
Copy link
Member

@kassens kassens left a comment

Choose a reason for hiding this comment

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

Please also update CI so this is checked, I think all that's needed is this glob:
https://github.com/facebook/react/blob/main/scripts/prettier/index.js#L54

@poteto
Copy link
Member Author

poteto commented Jun 20, 2024

Please also update CI so this is checked, I think all that's needed is this glob: https://github.com/facebook/react/blob/main/scripts/prettier/index.js#L54

I think you may have looked at the older commit, I already pushed those changes

@poteto poteto requested a review from kassens June 20, 2024 16:23
@kassens
Copy link
Member

kassens commented Jun 20, 2024

The linked glob '**/*.js' doesn't match *.ts and *.tsx files, right? Does yarn prettier-all format ts files?

Copy link
Member

@kassens kassens left a comment

Choose a reason for hiding this comment

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

see above

@poteto
Copy link
Member Author

poteto commented Jun 20, 2024

@kassens I think there's some misunderstanding. the compiler has its own prettier config if you look at the compiler directory. This particular top level script doesn't need to run on the compiler directory as a result and so the current setup to only run on .js files and ignore compiler is correct. Keep in mind this script only runs in CI.

But leaving the compiler entry in .prettierignore is disabling prettier itself in the editor if you open the root directory instead of just the compiler. Additionally it would try to run prettier on some .js files in compiler. So essentially the right thing to do is to remove the compiler from .prettierignore (allowing the editor to invoke prettier) and continue skipping it in the prettier script which only runs in CI.

Copy link
Member

@kassens kassens left a comment

Choose a reason for hiding this comment

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

I see, would be nice to integrate into the main prettier CI now that Forget is merged.

@poteto poteto merged commit a78be41 into gh/poteto/16/base Jun 20, 2024
44 checks passed
poteto added a commit that referenced this pull request Jun 20, 2024
Now that the compiler directory has its own prettier config, we can
remove the prettierignore entry for compiler/ so it still runs in your
editor if you open the root directory

ghstack-source-id: 5e3bd597cf2f11a9931f084eb909ffd81ebdca81
Pull Request resolved: #29993
@poteto poteto deleted the gh/poteto/16/head branch June 20, 2024 23:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed React Core Team Opened by a member of the React Core Team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants