Upgrade wp-prettier to v3.0.3 #54539
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jsnajdr
requested review from
noahtallen and
ObliviousHarmony
as code owners
jsnajdr
requested review from
t-hamano,
ajitbohra,
kevin940726,
Mamaduka,
ntwb,
nerrad,
ryanwelcher,
getdave and
ellatrix
as code owners
Mamaduka
approved these changes
Sep 18, 2023
jsnajdr
commented
Sep 18, 2023
| string | undefined | ||
| number | string | undefined , | ||
| number | undefined , | ||
| string | undefined , |
There was a problem hiding this comment.
This is wrong, formatting union types in tuples needs fixing.
|
Size Change: +43 B (0%) Total Size: 1.62 MB
ℹ️ View Unchanged
|
gziolo
approved these changes
Sep 18, 2023
There was a problem hiding this comment.
The changes applied to the ESLint plugin, WP scripts and the Gutenberg plugin look good.
It would be great to get also feedback regarding the formatting changes applied to the codebase. As a follow-up, we could list the final commit as ignored in git blame, see https://github.com/WordPress/gutenberg/blob/trunk/.git-blame-ignore-revs. Although, it's also fine to skip it this time as the changes aren't impactful.
| @@ -195,6 +195,7 @@ | |||
| "eslint-plugin-import": "2.25.2", | |||
| "eslint-plugin-jest": "27.2.3", | |||
| "eslint-plugin-jest-dom": "5.0.2", | |||
| "eslint-plugin-prettier": "5.0.0", | |||
There was a problem hiding this comment.
I don't think we need it anymore here after upgrading the lock file. Although if there is a reason to keep it, it's fine.
There was a problem hiding this comment.
I had to add this to the root package.json because otherwise the plugin got installed only into the /packages/eslint-plugin/node_modules subdirectory, not to the root node_modules. For some reason, even the new npm client didn't hoist the package, although it could, the slot is available, there is no second version of the same package that would conflict. And the wp-scripts lint-js command fails when the ESLint plugins are not resolvable from the root directory.
There was a problem hiding this comment.
Based on my experience, it's an issue with npm in general. After the lock file gets correctly updated, if you remove the dependency from package.json, it should remain hoisted. However, I don't guarantee it. It works in most of the cases 😄
There was a problem hiding this comment.
The sequence of events is like:
- install a new package. For some reason NPM installs it into a subdirectory instead of hoisting.
- install it again, this time also pinning it in the root
package.json. NPM is forced to hoist it. - remove it from the root
package.json. In the lockfile, the package is already in the hoisted position, and NPM won't move it -- there is no reason to move it, the hoisted position is fine. - when further changing the lockfile, the package remains in the hoisted position, because NPM respects the lockfile and the actual physical files in
node_modules, and doesn't move them unless it really has to.
Thanks for the reminder 👍 Yes, it needs to be a separate PR, after we know for sure what is the SHA of the |
|
Flaky tests detected in 4d9202d4de0e58727da09c912b7608fbb7562b6c. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/6220571870
|
tyxla
reviewed
Sep 18, 2023
| @@ -10,7 +10,7 @@ const UNITED_VALUE_REGEX = | |||
| */ | |||
| export function parseCSSUnitValue( | |||
| toParse: string | |||
| ): [ number | undefined, string | undefined ] { | |||
| ): [ number | undefined , string | undefined ] { | |||
There was a problem hiding this comment.
Seems like it's adding unwanted extra spaces in tuples, regarding of whether they're on multiple lines or not.
| @@ -182,7 +182,7 @@ export type ActionCreatorsOf< Config extends AnyConfig > = | |||
| // return type of each action creator to account for internal registry details -- | |||
| // for example, dispatched actions are wrapped with a Promise. | |||
| export type PromisifiedActionCreators< | |||
| ActionCreators extends MapOf< ActionCreator > | |||
| ActionCreators extends MapOf< ActionCreator >, | |||
There was a problem hiding this comment.
The comma after a single generic parameter definitely looks odd 🤔
There was a problem hiding this comment.
And yet it's perfectly logical, that's how trailingComma should work. The idea is when you add a second type parameter, it's a one-line diff, not two-line.
There was a problem hiding this comment.
That rationale makes sense to me 👍
jsnajdr
commented
Sep 18, 2023
| const ConditionalComponent = ifCondition( | ||
| ( props: Props ) => props.foo.length !== 0 | ||
| )( Component ); | ||
| const ConditionalComponent = ifCondition( ( props: Props ) => props.foo.length !== 0 )( Component ); |
There was a problem hiding this comment.
@tyxla Did you just manually format the .md file to create this change? Maybe we should automatically format all .md files.
There was a problem hiding this comment.
No, I formatted it automatically running npm run other:check-local-changes locally. No other changes were created.
There was a problem hiding this comment.
Re-added the commit since it seemed to be lost during the last rebase.
There was a problem hiding this comment.
Oh sorry, I did a rebase to include latest changes in trunk and this is what git push -f does 🙂
I added one more commit that formats some .md files, the ones that contained the call(/* foo */) formatting bug (missing spaces around the comment).
And also changelog entries for the eslint-plugin and scripts packages. The CI job demands also an entry for components, but that's not a reasonable request here, there are just formatting changes.
tyxla
reviewed
Sep 18, 2023
jsnajdr
force-pushed
the
upgrade/prettier-3.0.3
branch
from
51d9d7b
to
9fb33d6
Compare
jsnajdr
requested review from
juanmaguitar,
fabiankaegy and
ndiego
as code owners
This was referenced Sep 19, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Upgrades the
wp-prettierfork to version 3.0.3.Most visible formatting changes are:
( await x )expression had an extra trailing spacecall( /* none */ )) didn't have paren spacesconst x = y;) are now formatted more nicely, often breaking after the=How to test:
Go through the list of the formatting changes and report anything suspicious.