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

Upgrade prettier #26081

Merged
merged 5 commits into from
Jan 31, 2023
Merged

Upgrade prettier #26081

merged 5 commits into from
Jan 31, 2023

Conversation

kassens
Copy link
Member

@kassens kassens commented Jan 30, 2023

The old version of prettier we were using didn't support the Flow syntax to access properties in a type using SomeType['prop']. This updates prettier and rollup-plugin-prettier to the latest versions.

I added the prettier config arrowParens: "avoid" to reduce the diff size as the default has changed in Prettier 2.0. The largest amount of changes comes from function expressions now having a space. This doesn't have an option to preserve the old behavior, so we have to update this.

@facebook-github-bot facebook-github-bot added CLA Signed React Core Team Opened by a member of the React Core Team labels Jan 30, 2023
@kassens
Copy link
Member Author

kassens commented Jan 30, 2023

Note, I kept the formatting changes in a different commit from manual changes to help review.

@@ -37,7 +37,7 @@ const files = glob
.filter(f => !onlyChanged || changedFiles.has(f));

if (!files.length) {
return;
process.exit(0);
Copy link
Member Author

Choose a reason for hiding this comment

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

prettier didn't like the top level return. Used process.exit(0) instead to replicate the behavior (see line 82 is also using process.exit already).

Copy link
Collaborator

@gnoff gnoff left a comment

Choose a reason for hiding this comment

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

The config changes make sense. the code changes that I checked seem to make sense. What is the expected code review treatment for tool-updated code? I can go line by line but don't want to if not necessary cc @sebmarkbage

@kassens
Copy link
Member Author

kassens commented Jan 30, 2023

@gnoff I do a combination of:

  • review manual changes (that's why I separated them into separate git commits)
  • spot check automated changes that there's no unintended side effects

*/
'use strict';

import type {ErrorMap} from './Types';
Copy link
Member Author

Choose a reason for hiding this comment

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

This used a "type comment" before of the form:

/*::
[flow only code here]
*/

The flow prettier parser parses this as a pure AST w/o the comment and then when it's printed the comment is removed. Most of the scripts don't use types, I think we don't loose much by removing this here.

@@ -8,8 +8,8 @@ module.exports = {
jsxBracketSameLine: true,
trailingComma: 'es5',
printWidth: 80,
parser: 'babel',

parser: 'flow',
Copy link
Collaborator

Choose a reason for hiding this comment

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

should this be 'hermes' or 'flow'?

Copy link
Member Author

Choose a reason for hiding this comment

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

The hermes PR wasn't merged yet due to concerns of size IIRC. Looks like flow is current enough for the moment to parser everything. (not actually sure what the plan is going forward with hermes vs. flow parser).

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah I see

@react-sizebot
Copy link

Comparing: 1f5ce59...4da1450

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.min.js = 154.83 kB 154.83 kB = 49.11 kB 49.11 kB
oss-experimental/react-dom/cjs/react-dom.production.min.js = 156.83 kB 156.83 kB = 49.77 kB 49.77 kB
facebook-www/ReactDOM-prod.classic.js +0.12% 533.17 kB 533.79 kB +0.10% 94.97 kB 95.06 kB
facebook-www/ReactDOM-prod.modern.js +0.10% 518.27 kB 518.81 kB +0.11% 92.73 kB 92.83 kB

Significant size changes

Includes any change greater than 0.2%:

Expand to show
Name +/- Base Current +/- gzip Base gzip Current gzip
facebook-www/SchedulerPostTask-prod.classic.js +0.54% 4.07 kB 4.10 kB +0.53% 1.12 kB 1.13 kB
facebook-www/SchedulerPostTask-prod.modern.js +0.54% 4.07 kB 4.10 kB +0.53% 1.12 kB 1.13 kB
facebook-www/SchedulerPostTask-profiling.classic.js +0.54% 4.07 kB 4.10 kB +0.53% 1.12 kB 1.13 kB
facebook-www/SchedulerPostTask-profiling.modern.js +0.54% 4.07 kB 4.10 kB +0.53% 1.12 kB 1.13 kB
facebook-react-native/react/cjs/React-prod.js +0.38% 20.75 kB 20.83 kB +0.12% 5.18 kB 5.18 kB
facebook-www/React-prod.modern.js +0.37% 21.08 kB 21.16 kB +0.06% 5.28 kB 5.28 kB
facebook-react-native/react/cjs/React-profiling.js +0.37% 21.36 kB 21.44 kB +0.13% 5.30 kB 5.31 kB
facebook-www/React-prod.classic.js +0.36% 21.37 kB 21.44 kB +0.09% 5.35 kB 5.36 kB
facebook-www/React-profiling.modern.js +0.36% 21.69 kB 21.76 kB +0.07% 5.40 kB 5.41 kB
facebook-www/React-profiling.classic.js +0.35% 21.97 kB 22.05 kB +0.09% 5.48 kB 5.49 kB
oss-experimental/react-dom/client.js +0.32% 0.62 kB 0.62 kB +0.35% 0.29 kB 0.29 kB
oss-stable-semver/react-dom/client.js +0.32% 0.62 kB 0.62 kB +0.35% 0.29 kB 0.29 kB
oss-stable/react-dom/client.js +0.32% 0.62 kB 0.62 kB +0.35% 0.29 kB 0.29 kB
facebook-react-native/react-is/cjs/ReactIs-prod.js +0.31% 4.78 kB 4.80 kB = 1.15 kB 1.15 kB
facebook-react-native/react-is/cjs/ReactIs-profiling.js +0.31% 4.78 kB 4.80 kB = 1.15 kB 1.15 kB
facebook-www/ReactIs-prod.classic.js +0.28% 5.35 kB 5.36 kB +0.08% 1.30 kB 1.31 kB
facebook-www/ReactIs-prod.modern.js +0.28% 5.35 kB 5.36 kB +0.08% 1.30 kB 1.31 kB
facebook-react-native/scheduler/cjs/SchedulerMock-prod.js +0.25% 12.28 kB 12.31 kB +0.10% 2.88 kB 2.88 kB
facebook-react-native/scheduler/cjs/Scheduler-prod.js +0.25% 10.46 kB 10.49 kB +0.15% 2.64 kB 2.64 kB
facebook-www/SchedulerMock-prod.classic.js +0.24% 12.56 kB 12.59 kB +0.07% 2.97 kB 2.97 kB
facebook-www/SchedulerMock-prod.modern.js +0.24% 12.56 kB 12.59 kB +0.07% 2.97 kB 2.97 kB
facebook-react-native/scheduler/cjs/Scheduler-profiling.js +0.23% 11.07 kB 11.09 kB +0.07% 2.78 kB 2.78 kB
facebook-www/Scheduler-prod.classic.js +0.22% 11.77 kB 11.80 kB +0.14% 2.92 kB 2.92 kB
facebook-www/Scheduler-prod.modern.js +0.22% 11.77 kB 11.80 kB +0.14% 2.92 kB 2.92 kB

Generated by 🚫 dangerJS against 4da1450

@kassens kassens merged commit 6b30832 into facebook:main Jan 31, 2023
@kassens kassens deleted the upgrade-prettier branch January 31, 2023 13:25
github-actions bot pushed a commit that referenced this pull request Jan 31, 2023
The old version of prettier we were using didn't support the Flow syntax
to access properties in a type using `SomeType['prop']`. This updates
`prettier` and `rollup-plugin-prettier` to the latest versions.

I added the prettier config `arrowParens: "avoid"` to reduce the diff
size as the default has changed in Prettier 2.0. The largest amount of
changes comes from function expressions now having a space. This doesn't
have an option to preserve the old behavior, so we have to update this.

DiffTrain build for [6b30832](6b30832)
[View git log for this commit](https://github.com/facebook/react/commits/6b3083266686f62b29462d32de75c6e71f7ba3e3)
@gnoff
Copy link
Collaborator

gnoff commented Feb 6, 2023

@kassens When I try to annotate the type of this for functions the flow parser chokes but the babel parser still works

function myFunc (this: Thing) {}

let thing: Thing = {};
myFunc.call(thing);

This flow syntax is a little new to me, not sure when they introduced it but seems necessary without suppressing warnings and it's nice to have some type safety for the this argument. Is there a reason flow is better than babel for the prettier parser?

Also, I tried something like this with both flow and babel parsers and neither supported it

type X<T: ['one', 'two']> = {
  one: T[0],
  two: T[1]
}

Maybe this is still blocked on the hermes-flow parser patch?

facebook-github-bot pushed a commit to facebook/react-native that referenced this pull request Feb 15, 2023
Summary:
This sync includes the following changes:
- **[86c8c8db7](facebook/react@86c8c8db7 )**: test: Don't retry flushActWork if flushUntilNextPaint threw ([#26121](facebook/react#26121)) //<Sebastian Silbermann>//
- **[64acd3918](facebook/react@64acd3918 )**: remove unguarded getRootNode call ([#26152](facebook/react#26152)) //<Josh Story>//
- **[71cace4d3](facebook/react@71cace4d3 )**: Migrate testRunner from jasmine2 to jest-circus ([#26144](facebook/react#26144)) //<Ming Ye>//
- **[c8510227c](facebook/react@c8510227c )**: Treat displayName as undefined ([#26148](facebook/react#26148)) //<Sebastian Markbåge>//
- **[55542bc73](facebook/react@55542bc73 )**: Update jest printBasicPrototype config ([#26142](facebook/react#26142)) //<Ming Ye>//
- **[6396b6641](facebook/react@6396b6641 )**: Model Float on Hoistables semantics ([#26106](facebook/react#26106)) //<Josh Story>//
- **[ef9f6e77b](facebook/react@ef9f6e77b )**: Enable passing Server References from Server to Client ([#26124](facebook/react#26124)) //<Sebastian Markbåge>//
- **[35698311d](facebook/react@35698311d )**: Update jest escapeString config ([#26140](facebook/react#26140)) //<Ming Ye>//
- **[6ddcbd4f9](facebook/react@6ddcbd4f9 )**: [flow] enable LTI inference mode ([#26104](facebook/react#26104)) //<Jan Kassens>//
- **[53b1f69ba](facebook/react@53b1f69ba )**: Implement unstable_getBoundingClientRect in RN Fabric refs ([#26137](facebook/react#26137)) //<Rubén Norte>//
- **[594093496](facebook/react@594093496 )**: Update to Jest 29 ([#26088](facebook/react#26088)) //<Ming Ye>//
- **[28fcae062](facebook/react@28fcae062 )**: Add support for SVG `transformOrigin` prop ([#26130](facebook/react#26130)) //<Aravind D>//
- **[3ff1540e9](facebook/react@3ff1540e9 )**: Prefer JSX in ReactNoop assertions (to combat out-of-memory test runs) ([#26127](facebook/react#26127)) //<Sebastian Silbermann>//
- **[01a0c4e12](facebook/react@01a0c4e12 )**: Add Edge Server Builds for workerd / edge-light ([#26116](facebook/react#26116)) //<Sebastian Markbåge>//
- **[f0cf832e1](facebook/react@f0cf832e1 )**: Update Flight Fixture to "use client" instead of .client.js ([#26118](facebook/react#26118)) //<Sebastian Markbåge>//
- **[03a216070](facebook/react@03a216070 )**: Rename "dom" fork to "dom-node" and "bun" fork to "dom-bun" ([#26117](facebook/react#26117)) //<Sebastian Markbåge>//
- **[4bf2113a1](facebook/react@4bf2113a1 )**: Revert "Move the Webpack manifest config to one level deeper ([#26083](facebook/react#26083))"  ([#26111](facebook/react#26111)) //<Sebastian Markbåge>//
- **[2ef24145e](facebook/react@2ef24145e )**: [flow] upgrade to 0.199.0 ([#26096](facebook/react#26096)) //<Jan Kassens>//
- **[922dd7ba5](facebook/react@922dd7ba5 )**: Revert the outer module object to an object ([#26093](facebook/react#26093)) //<Sebastian Markbåge>//
- **[9d111ffdf](facebook/react@9d111ffdf )**: Serialize Promises through Flight ([#26086](facebook/react#26086)) //<Sebastian Markbåge>//
- **[0ba4698c7](facebook/react@0ba4698c7 )**: Fix async test in React reconciler ([#26087](facebook/react#26087)) //<Ming Ye>//
- **[8c234c0de](facebook/react@8c234c0de )**: Move the Webpack manifest config to one level deeper ([#26083](facebook/react#26083)) //<Sebastian Markbåge>//
- **[977bccd24](facebook/react@977bccd24 )**: Refactor Flight Encoding ([#26082](facebook/react#26082)) //<Sebastian Markbåge>//
- **[d7bb524ad](facebook/react@d7bb524ad )**: [cleanup] Remove unused package jest-mock-scheduler ([#26084](facebook/react#26084)) //<Ming Ye>//
- **[6b3083266](facebook/react@6b3083266 )**: Upgrade prettier ([#26081](facebook/react#26081)) //<Jan Kassens>//
- **[1f5ce59dd](facebook/react@1f5ce59dd )**: [cleanup] fully roll out warnAboutSpreadingKeyToJSX ([#26080](facebook/react#26080)) //<Jan Kassens>//

Changelog:
[General][Changed] - React Native sync for revisions 48b687f...fccf3a9

jest_e2e[run_all_tests]

Reviewed By: rubennorte

Differential Revision: D43305607

fbshipit-source-id: 8da7567ca2a182f4be27788935c2da30a731f83b
OlimpiaZurek pushed a commit to OlimpiaZurek/react-native that referenced this pull request May 22, 2023
Summary:
This sync includes the following changes:
- **[86c8c8db7](facebook/react@86c8c8db7 )**: test: Don't retry flushActWork if flushUntilNextPaint threw ([facebook#26121](facebook/react#26121)) //<Sebastian Silbermann>//
- **[64acd3918](facebook/react@64acd3918 )**: remove unguarded getRootNode call ([facebook#26152](facebook/react#26152)) //<Josh Story>//
- **[71cace4d3](facebook/react@71cace4d3 )**: Migrate testRunner from jasmine2 to jest-circus ([facebook#26144](facebook/react#26144)) //<Ming Ye>//
- **[c8510227c](facebook/react@c8510227c )**: Treat displayName as undefined ([facebook#26148](facebook/react#26148)) //<Sebastian Markbåge>//
- **[55542bc73](facebook/react@55542bc73 )**: Update jest printBasicPrototype config ([facebook#26142](facebook/react#26142)) //<Ming Ye>//
- **[6396b6641](facebook/react@6396b6641 )**: Model Float on Hoistables semantics ([facebook#26106](facebook/react#26106)) //<Josh Story>//
- **[ef9f6e77b](facebook/react@ef9f6e77b )**: Enable passing Server References from Server to Client ([facebook#26124](facebook/react#26124)) //<Sebastian Markbåge>//
- **[35698311d](facebook/react@35698311d )**: Update jest escapeString config ([facebook#26140](facebook/react#26140)) //<Ming Ye>//
- **[6ddcbd4f9](facebook/react@6ddcbd4f9 )**: [flow] enable LTI inference mode ([facebook#26104](facebook/react#26104)) //<Jan Kassens>//
- **[53b1f69ba](facebook/react@53b1f69ba )**: Implement unstable_getBoundingClientRect in RN Fabric refs ([facebook#26137](facebook/react#26137)) //<Rubén Norte>//
- **[594093496](facebook/react@594093496 )**: Update to Jest 29 ([facebook#26088](facebook/react#26088)) //<Ming Ye>//
- **[28fcae062](facebook/react@28fcae062 )**: Add support for SVG `transformOrigin` prop ([facebook#26130](facebook/react#26130)) //<Aravind D>//
- **[3ff1540e9](facebook/react@3ff1540e9 )**: Prefer JSX in ReactNoop assertions (to combat out-of-memory test runs) ([facebook#26127](facebook/react#26127)) //<Sebastian Silbermann>//
- **[01a0c4e12](facebook/react@01a0c4e12 )**: Add Edge Server Builds for workerd / edge-light ([facebook#26116](facebook/react#26116)) //<Sebastian Markbåge>//
- **[f0cf832e1](facebook/react@f0cf832e1 )**: Update Flight Fixture to "use client" instead of .client.js ([facebook#26118](facebook/react#26118)) //<Sebastian Markbåge>//
- **[03a216070](facebook/react@03a216070 )**: Rename "dom" fork to "dom-node" and "bun" fork to "dom-bun" ([facebook#26117](facebook/react#26117)) //<Sebastian Markbåge>//
- **[4bf2113a1](facebook/react@4bf2113a1 )**: Revert "Move the Webpack manifest config to one level deeper ([facebook#26083](facebook/react#26083))"  ([facebook#26111](facebook/react#26111)) //<Sebastian Markbåge>//
- **[2ef24145e](facebook/react@2ef24145e )**: [flow] upgrade to 0.199.0 ([facebook#26096](facebook/react#26096)) //<Jan Kassens>//
- **[922dd7ba5](facebook/react@922dd7ba5 )**: Revert the outer module object to an object ([facebook#26093](facebook/react#26093)) //<Sebastian Markbåge>//
- **[9d111ffdf](facebook/react@9d111ffdf )**: Serialize Promises through Flight ([facebook#26086](facebook/react#26086)) //<Sebastian Markbåge>//
- **[0ba4698c7](facebook/react@0ba4698c7 )**: Fix async test in React reconciler ([facebook#26087](facebook/react#26087)) //<Ming Ye>//
- **[8c234c0de](facebook/react@8c234c0de )**: Move the Webpack manifest config to one level deeper ([facebook#26083](facebook/react#26083)) //<Sebastian Markbåge>//
- **[977bccd24](facebook/react@977bccd24 )**: Refactor Flight Encoding ([facebook#26082](facebook/react#26082)) //<Sebastian Markbåge>//
- **[d7bb524ad](facebook/react@d7bb524ad )**: [cleanup] Remove unused package jest-mock-scheduler ([facebook#26084](facebook/react#26084)) //<Ming Ye>//
- **[6b3083266](facebook/react@6b3083266 )**: Upgrade prettier ([facebook#26081](facebook/react#26081)) //<Jan Kassens>//
- **[1f5ce59dd](facebook/react@1f5ce59dd )**: [cleanup] fully roll out warnAboutSpreadingKeyToJSX ([facebook#26080](facebook/react#26080)) //<Jan Kassens>//

Changelog:
[General][Changed] - React Native sync for revisions 48b687f...fccf3a9

jest_e2e[run_all_tests]

Reviewed By: rubennorte

Differential Revision: D43305607

fbshipit-source-id: 8da7567ca2a182f4be27788935c2da30a731f83b
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