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

Scripts: Improve recommended settings included in the package #17027

Merged
merged 5 commits into from
Aug 23, 2019

Conversation

gziolo
Copy link
Member

@gziolo gziolo commented Aug 13, 2019

Description

Fixes #17016.
Closes #17061.

Known Issues

When running npm run lint-js I see the following errors:

/Users/gziolo/PhpstormProjects/gutenberg-examples/test/examples.js
  10:1  error  'test' is not defined    no-undef
  11:2  error  'expect' is not defined  no-undef
  14:1  error  'test' is not defined    no-undef
  15:2  error  'expect' is not defined  no-undef

✖ 4 problems (4 errors, 0 warnings)

To solve it I had to add:

+ /* global test, expect */

at the top of the file which should work out of the box. This should work out of the box when using @wordpress/scripts.

I also had to put:

+ /**
+  * WordPress dependencies
+  */
import { Button } from '@wordpress/components';

above the import statement to fix the following issue:

 1:1  error  Expected preceding "WordPress dependencies" comment block  @wordpress/dependency-group

which I don't think we want to enforce outside of Gutenberg.

WordPress/gutenberg-examples#84 (review)

I think that having @wordpress/components as a dependency brings @wordpress/element as well. Ideally, we bring it as part of @wordpress/babel-preset-default which is the default handler for React regardless.

Changes Applied

@wordpress/babel-preset-default

Bug fix:

  • Added missing @wordpress/element dependency which is used internally.

@wordpress/eslint-plugin

Breaking changes:

  • New ruleset test-e2e added for end-to-end tests validation.
  • New ruleset test-unit added for unit tests validation.

Enhancement:

  • Remove @wordpress/dependency-group and @wordpress/gutenberg-phase rules from the custom and recommended configs and leave them as opt-in features.

All those changes directly influence @wordpress/scripts and improves the experience around its setup.

How has this been tested?

npm run lint-js

Types of changes

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.

@gziolo gziolo added [Type] Bug An existing feature does not function as intended [Status] In Progress Tracking issues with work in progress [Tool] WP Scripts /packages/scripts [Tool] Babel plugin import JSX pragma /packages/babel-plugin-import-jsx-pragma [Tool] ESLint plugin /packages/eslint-plugin labels Aug 13, 2019
@gziolo gziolo self-assigned this Aug 13, 2019
@gziolo gziolo changed the title Fix/wp scripts recommended settings Scripts: Improve recommended settings included in the package Aug 13, 2019
@gziolo gziolo force-pushed the fix/wp-scripts-recommended-settings branch from add25d0 to 8a8c320 Compare August 14, 2019 08:41
@gziolo gziolo requested a review from dmsnell as a code owner August 14, 2019 08:41
@gziolo gziolo removed the [Status] In Progress Tracking issues with work in progress label Aug 14, 2019
@gziolo gziolo requested a review from iandunn August 14, 2019 09:06
@@ -14,4 +14,26 @@ module.exports = {
document: true,
wp: 'readonly',
},
overrides: [
Copy link
Member Author

Choose a reason for hiding this comment

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

@ntwb, I was referring to this changes in #17061 (comment). I linked wrong PR 😃

@gziolo
Copy link
Member Author

gziolo commented Aug 22, 2019

@youknowriad - can we include this change in Gutenberg 6.4 release so we could batch all breaking changes related to @wordpress/scripts package and all its dependencies?

@youknowriad
Copy link
Contributor

Sure, I'd appreciate a review from someone actually using the scripts package. Maybe @swissspidy @ntwb or @nerrad

Copy link
Contributor

@nerrad nerrad left a comment

Choose a reason for hiding this comment

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

I had a question but on the whole these changes look good.

"core-js": "^3.1.4"
},
"peerDependencies": {
"@babel/core": "^7.0.0"
},
Copy link
Contributor

Choose a reason for hiding this comment

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

So does this change essentially mean that packages consuming wp-script don't need @babel/core as a dependency anymore?

Copy link
Member Author

Choose a reason for hiding this comment

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

At some point, it was added as a regular dependencies so it is redundant here.


### Enhancements

- Remove `@wordpress/dependency-group` and `@wordpress/gutenberg-phase` rules from the `custom` and `recommended` configs and leave them as opt-in features.
Copy link
Contributor

Choose a reason for hiding this comment

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

👍 This is mostly a breaking change for consumers that are using these rules. Most consumers won't want/need them so I like.

Copy link
Member Author

Choose a reason for hiding this comment

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

Do you think it should be highlighted as breaking change? I wasn’t sure where to put it. Given that, there are other breaking changes, we can more freely move it if necessary 😃

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure either. The scope of when it's considered a breaking change is fairly narrow and it's only breaking in the sense the rules won't be applied automatically anymore.

@gziolo gziolo merged commit 1a6d1b7 into master Aug 23, 2019
@gziolo gziolo deleted the fix/wp-scripts-recommended-settings branch August 23, 2019 04:57
@gziolo gziolo added this to the Gutenberg 6.4 milestone Aug 23, 2019
donmhico pushed a commit to donmhico/gutenberg that referenced this pull request Aug 27, 2019
…ess#17027)

* Babel Preset Default: Add missing @wordpress/element dependency

* Remove `@wordpress/dependency-group` and `@wordpress/gutenberg-phase` rules from the `custom` and `recommended` configs and leave them as opt-in features.

* ESLint Plugin: Extract 2 test configs and add them conditionally to the recommended one

* Add missing documentation for changes applied

* Scripts: Update CHANGELOG file
gziolo added a commit that referenced this pull request Aug 29, 2019
* Babel Preset Default: Add missing @wordpress/element dependency

* Remove `@wordpress/dependency-group` and `@wordpress/gutenberg-phase` rules from the `custom` and `recommended` configs and leave them as opt-in features.

* ESLint Plugin: Extract 2 test configs and add them conditionally to the recommended one

* Add missing documentation for changes applied

* Scripts: Update CHANGELOG file
gziolo added a commit that referenced this pull request Aug 29, 2019
* Babel Preset Default: Add missing @wordpress/element dependency

* Remove `@wordpress/dependency-group` and `@wordpress/gutenberg-phase` rules from the `custom` and `recommended` configs and leave them as opt-in features.

* ESLint Plugin: Extract 2 test configs and add them conditionally to the recommended one

* Add missing documentation for changes applied

* Scripts: Update CHANGELOG file
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Tool] Babel plugin import JSX pragma /packages/babel-plugin-import-jsx-pragma [Tool] ESLint plugin /packages/eslint-plugin [Tool] WP Scripts /packages/scripts [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add eslint-plugin-jest to @wordpress/scripts Provide unit testing devDependencies from wp-scripts
3 participants