Lint: Add rule to prohibit @wordpress/* unstable and experimental features #47712

sirreal · 2020-11-24T16:52:52Z

Changes proposed in this Pull Request

Introduce a new lint rule. It bans __experimental- and __unstable-prefixed imported names from @wordpress/* packages.

Experimental features caused problem recently (p58i-9K9-p2).

__unstable cannot be configured
__experimental can be configured by providing a configuration object: { "@wordpress/foo": [ "__experimentalBar" ] } allows the __experimentalBar import from @wordpress/foo package, e.g. import { __experimentalBar } from '@wordpress/foo';.

Adds this to the default wpcalypso rule set. Disable the rule for the only experimental feature we have e2e tests for.

Of course, the rule can always be disabled at the eslint level case-by-case.

Results

  ✘  https://google.com/#q=wpcalypso%2Fwp-no-unsafe-features

     Usage of `__experimentalInserterMenuExtension` from `@wordpress/block-editor` is not allowed


     apps/editing-toolkit/editing-toolkit-plugin/block-inserter-modifications/contextual-tips.js:10:10
      8 |  */
      9 | import { useState } from '@wordpress/element';
   > 10 | import { __experimentalInserterMenuExtension as InserterMenuExtension } from '@wordpress/block-editor';
        |          ^
     11 | import { registerPlugin } from '@wordpress/plugins';
     12 |
     13 | /**

  ✘  https://google.com/#q=wpcalypso%2Fwp-no-unsafe-features

     Usage of `__experimentalGetSettings` from `@wordpress/date` is not allowed


     apps/editing-toolkit/editing-toolkit-plugin/event-countdown-block/blocks/src/edit.js:8:20
      6 |  * WordPress dependencies
      7 |  */
   >  8 | import { dateI18n, __experimentalGetSettings } from '@wordpress/date';
        |                    ^
      9 | import { __ } from '@wordpress/i18n';
     10 | import { Button, DateTimePicker, Dropdown, Placeholder } from '@wordpress/components';
     11 |

  ✘  https://google.com/#q=wpcalypso%2Fwp-no-unsafe-features

     Usage of `__experimentalGetSettings` from `@wordpress/date` is not allowed


     apps/editing-toolkit/editing-toolkit-plugin/newspack-blocks/synced-newspack-blocks/blocks/carousel/edit.js:13:20
     11 |  */
     12 | import { __ } from '@wordpress/i18n';
   > 13 | import { dateI18n, __experimentalGetSettings } from '@wordpress/date';
        |                    ^
     14 | import { Component, createRef, Fragment } from '@wordpress/element';
     15 | import { InspectorControls } from '@wordpress/editor';
     16 | import {

  ✘  https://google.com/#q=wpcalypso%2Fwp-no-unsafe-features

     Usage of `__experimentalGetSettings` from `@wordpress/date` is not allowed


     apps/editing-toolkit/editing-toolkit-plugin/newspack-blocks/synced-newspack-blocks/blocks/homepage-articles/edit.js:25:20
     23 |  */
     24 | import { __ } from '@wordpress/i18n';
   > 25 | import { dateI18n, __experimentalGetSettings } from '@wordpress/date';
        |                    ^
     26 | import { Component, Fragment, RawHTML } from '@wordpress/element';
     27 | import {
     28 |       BlockControls,

  ✘  https://google.com/#q=wpcalypso%2Fwp-no-unsafe-features

     Usage of `__experimentalBlock` from `@wordpress/block-editor` is not allowed


     apps/editing-toolkit/editing-toolkit-plugin/premium-content/blocks/buttons/edit.js:5:23
     3 |  */
     4 | // eslint-disable-next-line wpcalypso/import-docblock
   > 5 | import { InnerBlocks, __experimentalBlock as Block } from '@wordpress/block-editor';
       |                       ^
     6 | import { compose } from '@wordpress/compose';
     7 | import { withDispatch, withSelect } from '@wordpress/data';
     8 | import { useEffect } from '@wordpress/element';

  ✘  https://google.com/#q=wpcalypso%2Fwp-no-unsafe-features

     Usage of `__experimentalBlock` from `@wordpress/block-editor` is not allowed


     apps/editing-toolkit/editing-toolkit-plugin/premium-content/blocks/login-button/edit.js:12:39
     10 | import { useCallback } from '@wordpress/element';
     11 | import { PanelBody, RangeControl } from '@wordpress/components';
   > 12 | import { InspectorControls, RichText, __experimentalBlock as Block } from '@wordpress/block-editor';
        |                                       ^
     13 |
     14 | /**
     15 |  * Internal dependencies

  ✘  https://google.com/#q=wpcalypso%2Fwp-no-unsafe-features

     Usage of `__experimentalBlock` from `@wordpress/block-editor` is not allowed


     apps/editing-toolkit/editing-toolkit-plugin/premium-content/index.js:11:10
      9 | import apiFetch from '@wordpress/api-fetch';
     10 | import { getBlockType, registerBlockType, unregisterBlockType } from '@wordpress/blocks';
   > 11 | import { __experimentalBlock } from '@wordpress/block-editor';
        |          ^
     12 |
     13 | /**
     14 |  * Internal dependencies

  ✘  https://google.com/#q=wpcalypso%2Fwp-no-unsafe-features

     Usage of `__experimentalMainDashboardButton` from `@wordpress/interface` is not allowed


     apps/wpcom-block-editor/src/calypso/features/iframe-bridge-server.js:13:10
     11 | import { addQueryArgs, getQueryArg } from '@wordpress/url';
     12 | import { registerPlugin } from '@wordpress/plugins';
   > 13 | import { __experimentalMainDashboardButton as MainDashboardButton } from '@wordpress/interface';
        |          ^
     14 | import { Button } from '@wordpress/components';
     15 | import { wordpress } from '@wordpress/icons';
     16 | import { Component, useEffect, useState } from 'react';

  ✘  https://google.com/#q=wpcalypso%2Fwp-no-unsafe-features

     Usage of `__experimentalInserterMenuExtension` from `@wordpress/block-editor` is not allowed


     apps/wpcom-block-editor/src/wpcom/features/tracking/wpcom-inserter-menu-search-term.js:11:10
      9 | import { useState } from '@wordpress/element';
     10 | import { useSelect } from '@wordpress/data';
   > 11 | import { __experimentalInserterMenuExtension as InserterMenuExtension } from '@wordpress/block-editor';
        |          ^
     12 |
     13 | /**
     14 |  * Internal dependencies

✘ 9 problems (9 errors, 0 warnings)

Testing instructions

Just the editing toolkit: npx eslint-nibble --rule wpcalypso/wp-no-unsafe-features apps/editing-toolkit
Most everything (slow!): npx eslint-nibble --rule wpcalypso/wp-no-unsafe-features apps client packages

Closes #47551

matticbot · 2020-11-24T16:52:54Z

Test live: https://calypso.live/?branch=add/eslint-experimental-features

matticbot · 2020-11-24T17:07:37Z

This PR does not affect the size of JS and CSS bundles shipped to the user's browser.

Generated by performance advisor bot at iscalypsofastyet.com.

packages/eslint-plugin-wpcalypso/lib/rules/test/wp-no-unsafe-features.js

scinos · 2020-11-25T13:58:02Z

packages/eslint-plugin-wpcalypso/package.json

@@ -26,5 +26,8 @@
 	},
 	"devDependencies": {
 		"babel-eslint": "^10.1.0"
+	},
+	"scripts": {
+		"test": "jest"


Shouldn't this be tested as part of yarn test-packages in the root? Why do we need our own jest call?

I moved into the package and did yarn test. I expected that to work and was surprised when it didn't. With this change it does work, but if that's in violation of the repo setup I'm happy to drop the change.

I think having a test command here may be a bit confusing:

Technically we don't run this command on CI, so it can break at anytime and go unnoticed. In CI we only do yarn test-packages in the root.

With the current setup, the way to run tests for a single package is yarn test-packages packages/eslint-plugin-wpcalypso. Doing cd packages/eslint-plugin-wpcalypso && yarn jest also works, but that's a trick

scinos · 2020-11-25T14:02:26Z

I changed the 'Code style' build to lint all files and not only the files changed in this PR. This should show existing violations of this rule (if any)

scinos · 2020-11-25T14:22:47Z

Existing violations in the codebase

I think we should "fix" them before merging this new rule. And by "fix" I mean either remove the usage, or if too hard/expensive add an explicit //eslint-disable-next-line for that specific import.

sirreal · 2020-11-25T16:16:38Z

Thanks for the review @scinos 👍

I think we should "fix" them before merging this new rule.

Fixing these is on the short list, especially considering they caused a production issue recently.

Teams use these features because they needed them and the features are fine as long as the Gutenberg version we're using doesn't change. We have control over the Gutenberg version, so what we want to do is make sure we have tests on the features so they don't break without notice. For most (all?) of the features, we can probably add a test and configure the rule:

wp-calypso/test/e2e/specs/wp-gutenberg-experimental-features-spec.js

Lines 21 to 30 in d278aee

    
           // Experimental Gutenberg features that we depend on in Calypso (and other projects) 
        
           // Tests will fail if an experimental feature is no longer being exported from one 
        
           // of the @wordpress/* packages. The purpose of these tests is to give us an early 
        
           // warning if an experimental feature has been removed or renamed. 
        
           const EXPERIMENTAL_FEATURES = { 
        
           	'@wordpress/block-editor': [ 
        
           		// Used in the premium content block in the Editing Toolkit plugin 
        
           		'__experimentalBlock', 
        
           	], 
        
           };

https://github.com/Automattic/wp-calypso/blob/b311e2bb1f08bf82fe36f145ac9272863b4bc14e/.eslintrc.js#L392-L395

I'll see what that looks like, if it's really that simple we can do it here. Otherwise I'd prefer to get the responsible teams to handle each case.

sirreal · 2020-11-25T16:35:09Z

packages/eslint-config-wpcalypso/index.js

+		'wpcalypso/jsx-gridicon-size': 'error',
+		'wpcalypso/jsx-classname-namespace': 'error',
+		'wpcalypso/redux-no-bound-selectors': 'error',
+		'wpcalypso/wp-no-unsafe-features': 'error',


This line adds the new rule. The rest just rewrite from confusing numbers to clear strings.

fullofcaffeine · 2020-11-25T23:10:11Z

Great work here 💯!

I've tried it out today and have a few questions:

1) Running npx eslint-nibble --rule wpcalypso/wp-no-unsafe-features apps client packages locally didn't yield any lint errors for me:

~/workspace/code/wp-calypso (add/eslint-experimental-features●)$ npx eslint-nibble --rule wpcalypso/wp-no-unsafe-features apps client packages                                                          [system] 
Invalid import, only literal imports are supported. File /home/fullofcaffeine/workspace/code/wp-calypso/client/components/localized-moment/provider.js:35                                                                                                     
Invalid import, only literal imports are supported. File /home/fullofcaffeine/workspace/code/wp-calypso/client/lib/flags/index.js:9                                                                                                                           
Invalid import, only literal imports are supported. File /home/fullofcaffeine/workspace/code/wp-calypso/client/signup/config/step-components.js:78                                                                                                            
Invalid import, only literal imports are supported. File /home/fullofcaffeine/workspace/code/wp-calypso/packages/calypso-codemods/api.js:13                                                                                                                   
Invalid import, only literal imports are supported. File /home/fullofcaffeine/workspace/code/wp-calypso/packages/effective-module-tree/index.js:68                                                                                                            
Invalid import, only literal imports are supported. File /home/fullofcaffeine/workspace/code/wp-calypso/packages/effective-module-tree/index.js:114

No lint failures found for rule(s): wpcalypso/wp-no-unsafe-features
Try running again without "--rule"

Am I missing something?

2) Should we also detect when unstable members are used, like this?

packages/eslint-plugin-wpcalypso/docs/rules/wp-no-unsafe-features.md

sirreal · 2020-11-26T14:08:19Z

Proposing for Gutenberg: WordPress/gutenberg#27301

0 -> "off" 1 -> "warn" 2 -> "error"

Based on improvements in WordPress/gutenberg#27301

sirreal · 2020-11-26T15:41:04Z

test/e2e/specs/wp-gutenberg-experimental-features-spec.js

@@ -23,10 +23,21 @@ const gutenbergUser =
 // of the @wordpress/* packages. The purpose of these tests is to give us an early
 // warning if an experimental feature has been removed or renamed.
 const EXPERIMENTAL_FEATURES = {


It's unclear whether and when this is actually run. I can see the spec file in CircleCI logs, but I don't see the describe actually appear like it isn't being run. We may need to tag this describe somehow, e.g. @parallel and/or whatever tag runs on Gutenberg edge tests.

@fullofcaffeine can own that bit? I'd expect this to run as part of the full suite and the Gutenberg edge suites. We can handle that in a separate PR (which could include the changes to this file).

@fullofcaffeine can own that bit?

Sure! Tracked it here. Will get to it as soon as I can.

packages/eslint-plugin-wpcalypso/docs/rules/no-unsafe-wp-apis.md

sirreal · 2020-11-27T10:22:00Z

packages/eslint-plugin-wpcalypso/docs/rules/no-unsafe-wp-apis.md

+Examples of **incorrect** code for this rule:
+
+```js
+/* eslint-disable wpcalypso/no-unsafe-wp-apis */


This seems like encouraging folks to disable the rule right in the docs. That seems like the wrong message. Can we ignore the lint error or add an .eslintrc override?

packages/eslint-plugin-wpcalypso/package.json

scinos · 2020-11-27T12:39:11Z

packages/eslint-plugin-wpcalypso/docs/rules/i18n-ellipsis.md

@@ -7,7 +7,7 @@ Three dots for indicating an ellipsis should be replaced with the UTF-8 characte
 The following patterns are considered warnings:

 ```js
-translate( 'Loading…' );
+translate( 'Loading...' );


Thanks for fixing this :)

sirreal · 2020-11-27T15:28:04Z

The Gutenberg edge failures are expected and the woo test failure is happening on all PRs.

I'll merge.

sirreal added the [Status] In Progress label Nov 24, 2020

sirreal mentioned this pull request Nov 25, 2020

Lint: Add eslint rule prohibiting experimental feature usage #47551

Closed

sirreal added the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Nov 25, 2020

sirreal marked this pull request as ready for review November 25, 2020 13:50

sirreal self-assigned this Nov 25, 2020

sirreal requested review from a team November 25, 2020 13:51

scinos reviewed Nov 25, 2020

View reviewed changes

packages/eslint-plugin-wpcalypso/lib/rules/test/wp-no-unsafe-features.js Outdated Show resolved Hide resolved

scinos reviewed Nov 25, 2020

View reviewed changes

sirreal added the [Status] Needs e2e Testing Gutenberg Edge Run e2e tests with a special site that is running the latest Gutenberg plugin snapshot label Nov 25, 2020

sirreal commented Nov 25, 2020

View reviewed changes

fullofcaffeine reviewed Nov 25, 2020

View reviewed changes

packages/eslint-plugin-wpcalypso/docs/rules/wp-no-unsafe-features.md Outdated Show resolved Hide resolved

sirreal force-pushed the add/eslint-experimental-features branch from e90d65b to e97061e Compare November 26, 2020 11:51

sirreal added 10 commits November 26, 2020 16:33

Add rule for experimental and unstable features

2592316

Add rule for experimental and unstable features

0b04f14

Fix rule name

8ad7769

Use descriptive level strings

312b78a

0 -> "off" 1 -> "warn" 2 -> "error"

Enable the wp-no-unsafe-features rule

06003cc

Improve rule meta

3f20525

Add config example

1d6fbdc

Allow the only tested rule

a68f322

Cleanup

cfad0ca

Add e2e tests and enable features

a5a5076

sirreal added 2 commits November 26, 2020 16:33

Update docs

3b74e81

Update rule

1cab570

Based on improvements in WordPress/gutenberg#27301

sirreal force-pushed the add/eslint-experimental-features branch from e97061e to 1cab570 Compare November 26, 2020 15:33

sirreal commented Nov 26, 2020

View reviewed changes

packages/eslint-plugin-wpcalypso/docs/rules/no-unsafe-wp-apis.md Outdated Show resolved Hide resolved

Fix example docs

4552cb2

sirreal commented Nov 27, 2020

View reviewed changes

packages/eslint-plugin-wpcalypso/package.json Outdated Show resolved Hide resolved

sirreal added 4 commits November 27, 2020 11:23

Remove package-specific test script

9924337

Disable via .eslintrc

100badf

Fix bad fix in error example

798d895

Simplify single message

aa97e6b

scinos reviewed Nov 27, 2020

View reviewed changes

scinos self-requested a review November 27, 2020 12:39

scinos approved these changes Nov 27, 2020

View reviewed changes

sirreal merged commit 6f28b72 into trunk Nov 27, 2020

sirreal deleted the add/eslint-experimental-features branch November 27, 2020 15:28

matticbot removed [Status] In Progress [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Nov 27, 2020

p-jackson mentioned this pull request Dec 14, 2020

Update experimental features tests #48164

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Lint: Add rule to prohibit @wordpress/* unstable and experimental features #47712

Lint: Add rule to prohibit @wordpress/* unstable and experimental features #47712

sirreal commented Nov 24, 2020 •

edited by fullofcaffeine

Loading

matticbot commented Nov 24, 2020

matticbot commented Nov 24, 2020

scinos Nov 25, 2020

sirreal Nov 25, 2020

scinos Nov 27, 2020

scinos commented Nov 25, 2020

scinos commented Nov 25, 2020

sirreal commented Nov 25, 2020

sirreal Nov 25, 2020

fullofcaffeine commented Nov 25, 2020 •

edited

Loading

sirreal commented Nov 26, 2020

sirreal Nov 26, 2020 •

edited

Loading

fullofcaffeine Nov 26, 2020

sirreal Nov 27, 2020

scinos Nov 27, 2020

sirreal commented Nov 27, 2020

Lint: Add rule to prohibit @wordpress/* unstable and experimental features #47712

Lint: Add rule to prohibit @wordpress/* unstable and experimental features #47712

Conversation

sirreal commented Nov 24, 2020 • edited by fullofcaffeine Loading

Changes proposed in this Pull Request

Testing instructions

matticbot commented Nov 24, 2020

matticbot commented Nov 24, 2020

scinos Nov 25, 2020

Choose a reason for hiding this comment

sirreal Nov 25, 2020

Choose a reason for hiding this comment

scinos Nov 27, 2020

Choose a reason for hiding this comment

scinos commented Nov 25, 2020

scinos commented Nov 25, 2020

sirreal commented Nov 25, 2020

sirreal Nov 25, 2020

Choose a reason for hiding this comment

fullofcaffeine commented Nov 25, 2020 • edited Loading

sirreal commented Nov 26, 2020

sirreal Nov 26, 2020 • edited Loading

Choose a reason for hiding this comment

fullofcaffeine Nov 26, 2020

Choose a reason for hiding this comment

sirreal Nov 27, 2020

Choose a reason for hiding this comment

scinos Nov 27, 2020

Choose a reason for hiding this comment

sirreal commented Nov 27, 2020

sirreal commented Nov 24, 2020 •

edited by fullofcaffeine

Loading

fullofcaffeine commented Nov 25, 2020 •

edited

Loading

sirreal Nov 26, 2020 •

edited

Loading