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

Prettier: Update format-js to use default config, and update editor docs usage #20036

Merged
merged 6 commits into from Feb 6, 2020

Conversation

@mkaz
Copy link
Member

mkaz commented Feb 4, 2020

Description

Updates format-js instead of failing if it does not find a default config, to use the default in the packages/scripts config. This supports using the script outside of the Gutenberg project where a default config might not already exist, for example developing a block plugin.

Updates documentation, clarifying editor usage, including part about workpsace settings in Visual Studio Code and adds link to other editor integrations.

How has this been tested?

  • Confirm prettier still runs as expected in Visual Studio Code.
  • Confirm npm run format-js works as expected
  • Use format-js script without a prettier config, and should still work

Types of changes

Updates format-js script moving the check to configure the default config and not fail.
Minor update to getting started docs.

mkaz added 2 commits Feb 4, 2020
@gziolo

This comment has been minimized.

Copy link
Member

gziolo commented Feb 4, 2020

It would be great to relax this check:

if ( ! hasProjectPrettierConfig ) {
return {
success: false,
message:
chalk.red(
'The Prettier config file was not found in your project\n'
) +
'You need to create a top-level Prettier config file in your project to get ' +
'automatic code formatting that works with IDE and editor integrations.\n\n',
};
}

If no Prettier config found, we can always defer to the default config provided in the config folder inside the package.

@mkaz mkaz requested review from nerrad, ntwb and youknowriad as code owners Feb 4, 2020
@mkaz

This comment has been minimized.

Copy link
Member Author

mkaz commented Feb 4, 2020

@gziolo Updates see it doesn't quit when no config, switched it to provide the config using the fromConfigRoot. This works when running the format-js script.

However, the pre-commit hook is using something else, eslint-plugin-prettier, so after deleting the .prettierrc.js file it uses the default config is incorrect

@gziolo

This comment has been minimized.

Copy link
Member

gziolo commented Feb 5, 2020

@gziolo Updates see it doesn't quit when no config, switched it to provide the config using the fromConfigRoot. This works when running the format-js script.

However, the pre-commit hook is using something else, eslint-plugin-prettier, so after deleting the .prettierrc.js file it uses the default config is incorrect

I think I have an idea how to fix it. You can provide options to the prettier/prettier rule. I hope we can load the config file and pass it as options when the prettier config is not detected. See: https://github.com/prettier/eslint-plugin-prettier#options

@gziolo

This comment has been minimized.

Copy link
Member

gziolo commented Feb 5, 2020

Quick and dirty but works:

diff --git a/packages/eslint-plugin/configs/recommended.js b/packages/eslint-plugin/configs/recommended.js
index 9e1659685..234d0dbbf 100644
--- a/packages/eslint-plugin/configs/recommended.js
+++ b/packages/eslint-plugin/configs/recommended.js
@@ -1,7 +1,22 @@
+// TODO: Use the actual function that detects whether Prettier config exists.
+const hasProjectPrettierConfig = false;
+const prettierOptions = hasProjectPrettierConfig
+       ? {}
+       : require( '../../scripts/config/.prettierrc' );
+
 module.exports = {
        extends: [
                require.resolve( './recommended-with-formatting.js' ),
                'plugin:prettier/recommended',
                'prettier/react',
        ],
+       rules: {
+               'prettier/prettier': [
+                       'error',
+                       prettierOptions,
+                       {
+                               usePrettierrc: false,
+                       },
+               ],
+       },
 };

Although, now that I think about it. Maybe we should apply this rule inside the config provided for @wordpress/scripts. This way ESLint would work properly there, but we still would have to keep .prettierrc.js proxy in the Gutenberg project. I must admit it's a complex issue to solve :)

@jsnajdr

This comment has been minimized.

Copy link
Contributor

jsnajdr commented Feb 5, 2020

It would be great to relax this check:

The reason I added this check is to make editor integrations more reliable. If you have a Prettier plugin in your editor and execute the "format with Prettier" command, the editor will search for Prettier config in the default location. The editor has no knowledge about the format-js script and about the Prettier config somewhere deep in node_modules/@wordpress/scripts/config.

To ensure that both format-js and vanilla Prettier run use the same config, I enforced a presence of the root config file and made format-js fail.

@mkaz

This comment has been minimized.

Copy link
Member Author

mkaz commented Feb 5, 2020

I don't think this PR is as necessary, the top-level .prettierrc.js is loading from scripts already using:

module.exports = require( '@wordpress/scripts/config/.prettierrc.js' );
@gziolo

This comment has been minimized.

Copy link
Member

gziolo commented Feb 5, 2020

I don't think this PR is as necessary, the top-level .prettierrc.js is loading from scripts already using:

module.exports = require( '@wordpress/scripts/config/.prettierrc.js' );

Yes, for Gutenberg it's all fine. As usual 😄

The issue is when you are using @wordpress/scripts with other projects. By default, ESLint will use all default settings to enforce style because the Prettier config is not there. It's not something dramatically wrong but it's different than what Gutenberg uses.

@mkaz

This comment has been minimized.

Copy link
Member Author

mkaz commented Feb 5, 2020

@gziolo Ok, so I think not deleting the top-level will fix the issue.
I think it was my misunderstanding of the initial request.

@gziolo

This comment has been minimized.

Copy link
Member

gziolo commented Feb 5, 2020

Yes, it fixes the issue because ESLint will use it in Gutenberg for linting. I guess it's enough to move forward and we can discuss the issue with wp-scripts lint-js separately as it's quite complex :)

mkaz added 2 commits Feb 5, 2020
… on workplace settings
@mkaz mkaz changed the title Docs: Update editor setup to include configuring prettier Prettier: Update format-js to use default config, and update editor docs usage Feb 5, 2020
@mkaz

This comment has been minimized.

Copy link
Member Author

mkaz commented Feb 5, 2020

@gziolo and @jsnajdr I update this PR description to clarify the goal.

@gziolo

This comment has been minimized.

Copy link
Member

gziolo commented Feb 6, 2020

The reason I added this check is to make editor integrations more reliable. If you have a Prettier plugin in your editor and execute the "format with Prettier" command, the editor will search for Prettier config in the default location. The editor has no knowledge about the format-js script and about the Prettier config somewhere deep in node_modules/@wordpress/scripts/config.

To ensure that both format-js and vanilla Prettier run use the same config, I enforced a presence of the root config file and made format-js fail.

It's fine but it makes it hard to use outside of Gutenberg. That's why @mkaz included detailed docs how to setup Prettier with Visual Studio Code and linked to Pretier docs for other editors. I admit it isn't perfect and your proposal makes a lot of sense. However, the issue is with wp-scripts lint-js that now depends on Prettier config as well 🙃 In addition, ESLint extension for VSC and other IDEs won't work out of the box with @wordpress/scripts and all linting script because of the same reasons. This PR brings some consistency, but I want us to continue the discussion on this topic and ensure we can provide the best experience possible.

The next steps after this PR lands would be to conditionally load the shared Prettier config for wp-scripts lint-js when there is no Prettier config found in the root of the project.

In addition, the check you implemented could be added as part of the script execution but maybe it should be more like the information shared or prompt to action that automates the process of IDE integration.

@gziolo
gziolo approved these changes Feb 6, 2020
Copy link
Member

gziolo left a comment

Let's wait also for @jsnajdr to ensure we are fine with the plan I outlined.

@jsnajdr
jsnajdr approved these changes Feb 6, 2020
Copy link
Contributor

jsnajdr left a comment

Yes, this looks correct 👍

@gziolo gziolo merged commit 57daacb into master Feb 6, 2020
1 check passed
1 check passed
Travis CI - Pull Request Build Passed
Details
@gziolo gziolo deleted the docs/prettier-config branch Feb 6, 2020
@github-actions github-actions bot added this to the Gutenberg 7.5 milestone Feb 6, 2020
@gziolo

This comment has been minimized.

Copy link
Member

gziolo commented Feb 6, 2020

I opened #20071 with the fix for wp-scripts lint-js to use the default Prettier config when no config found.

The last remaining task is to improve integration with IDEs. We probably should cover more commands:

  • format-js
  • lint-style
  • lint-js

I need to test with https://github.com/WordPress/gutenberg-examples.

chipsnyder added a commit that referenced this pull request Feb 7, 2020
…ocs usage (#20036)

* Add instructions for configuring VSCode to use scripts config

* Linting

* Dont fail on missing config, use default package config

* Restore .prettierrc.js

* Update docs, prettier path is not needed for setup. Added information on workplace settings

* heh, prettier formating
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.