Updating ESLint plugin version #2819

wjrosa · 2024-01-04T22:28:40Z

Changes proposed in this Pull Request:

This PR just updates the current version of @woocommerce/eslint-plugin package. This is to put back some of the rules we disabled here due to constant warnings. But, to avoid file conflicts with coming PRs (due to new rules alerts), some rules are being disabled temporarily.

Testing instructions

Code review should be enough.
Check if the tests are still passing.
Check if the linting task below worked. You can also checkout to this branch and run npm run lint:js

Covered with tests (or have a good reason not to test in description ☝️)
Added changelog entry in both changelog.txt and readme.txt (or does not apply)
Tested on mobile (or does not apply)

Post merge

Added testing instructions to the Release Testing Instructions wiki page (or does not apply)

Mayisha

Thanks @wjrosa. The changes look good to me and npm run lint:js passes perfectly. I have added two little recommendations but those are not a blocker.

.eslintrc.js

client/components/payment-method-fees-pill/index.js

… avoid conflicts with upcoming changes

asumaran · 2024-01-11T19:28:01Z

@wjrosa now that eslint is up to date I guess is fine to remove the // eslint-disable-next-line @wordpress/no-global-event-listener lines I've added in #2252?

asumaran · 2024-01-11T19:43:06Z

it seems the @wordpress/no-global-event-listener rule is still there:

/Users/asumaran/Developer/woocommerce-gateway-stripe/client/utils/use-confirm-navigation.js
  32:3  warning  Avoid using (add|remove)EventListener with globals. Use `ownerDocument` or `ownerDocument.defaultView` on a node ref instead  @wordpress/no-global-event-listener
  35:4  warning  Avoid using (add|remove)EventListener with globals. Use `ownerDocument` or `ownerDocument.defaultView` on a node ref instead  @wordpress/no-global-event-listener

wjrosa · 2024-01-11T20:06:59Z

it seems the @wordpress/no-global-event-listener rule is still there:

/Users/asumaran/Developer/woocommerce-gateway-stripe/client/utils/use-confirm-navigation.js
  32:3  warning  Avoid using (add|remove)EventListener with globals. Use `ownerDocument` or `ownerDocument.defaultView` on a node ref instead  @wordpress/no-global-event-listener
  35:4  warning  Avoid using (add|remove)EventListener with globals. Use `ownerDocument` or `ownerDocument.defaultView` on a node ref instead  @wordpress/no-global-event-listener

Yes, I just tried and got the same. Do you think we can refactor using the suggestion from the warning?

asumaran · 2024-01-11T20:24:09Z

Yes, I just tried and got the same. Do you think we can refactor using the suggestion from the warning?

I don't think so. I left a message in the PR about it

there's no element that we can use to get the defaultView object.

wjrosa · 2024-01-11T22:03:22Z

Yes, I just tried and got the same. Do you think we can refactor using the suggestion from the warning?

I don't think so. I left a message in the PR about it

there's no element that we can use to get the defaultView object.

I see. So I guess we need to stick with that rules-ignoring lines for a while then :(

asumaran · 2024-01-11T23:35:41Z

@wjrosa I was wondering why the rule is still present in eslint given the fact that the rule was removed from recent versions of @wordpress/eslint-plugin.

It seems eslint is picking up @wordpress/eslint-plugin@8.0.2 instead of @wordpress/eslint-plugin@11.1.0 which is set as direct dependency of @woocommerce/eslint-plugin@2.2.0.

Running eslint . --ext=js,jsx,ts,tsx --debug (npm run lint:js with --debug) will show what version of @wordpress/eslint-plugin is picking up:

- /Users/asumaran/Developer/woocommerce-gateway-stripe/node_modules/@woocommerce/eslint-plugin/index.js
2024-01-11T23:14:56.462Z eslintrc:config-array-factory Loaded: /Users/asumaran/Developer/woocommerce-gateway-stripe/node_modules/@woocommerce/eslint-plugin/configs/custom.js (/Users/asumaran/Developer/woocommerce-gateway-stripe/node_modules/@woocommerce/eslint-plugin/configs/custom.js)
2024-01-11T23:14:56.462Z eslintrc:config-array-factory Loading JS config file: /Users/asumaran/Developer/woocommerce-gateway-stripe/node_modules/@woocommerce/eslint-plugin/configs/custom.js
2024-01-11T23:14:56.462Z eslintrc:config-array-factory Loading plugin "@wordpress" from /Users/asumaran/Developer/woocommerce-gateway-stripe/node_modules/@woocommerce/eslint-plugin/configs/custom.js
2024-01-11T23:14:56.463Z eslintrc:config-array-factory Loaded: @wordpress/eslint-plugin@8.0.2 (/Users/asumaran/Developer/woocommerce-gateway-stripe/node_modules/@wordpress/eslint-plugin/index.js)

Notice that it's picking up the config file from the @wordpress/eslint-plugin@8.0.2 package -- which still contains the @wordpress/no-global-event-listener rule.

Not sure if package-lock.json is wrong or we need to fine tune .eslintrc.js to load @wordpress/eslint-plugin@11.1.0 which is the correct version eslint should use and doesn't have the aforementioned rule.

asumaran · 2024-01-12T00:58:19Z

@wjrosa I've forced eslint to use @wordpress/eslint-plugin@11.1.0 by adding the package as a dev dependency of WCStripe so it takes precedence over the @wordpress/eslint-plugin@8.0.2 by running:

npm install @wordpress/eslint-plugin@^11.0.0 -D

and it loaded the correct rules this time:

- /Users/asumaran/Developer/woocommerce-gateway-stripe/node_modules/@woocommerce/eslint-plugin/index.js
2024-01-12T00:41:29.007Z eslintrc:config-array-factory Loaded: /Users/asumaran/Developer/woocommerce-gateway-stripe/node_modules/@woocommerce/eslint-plugin/configs/custom.js (/Users/asumaran/Developer/woocommerce-gateway-stripe/node_modules/@woocommerce/eslint-plugin/configs/custom.js)
2024-01-12T00:41:29.007Z eslintrc:config-array-factory Loading JS config file: /Users/asumaran/Developer/woocommerce-gateway-stripe/node_modules/@woocommerce/eslint-plugin/configs/custom.js
2024-01-12T00:41:29.007Z eslintrc:config-array-factory Loading plugin "@wordpress" from /Users/asumaran/Developer/woocommerce-gateway-stripe/node_modules/@woocommerce/eslint-plugin/configs/custom.js
2024-01-12T00:41:29.008Z eslintrc:config-array-factory Loaded: @wordpress/eslint-plugin@11.1.0 (/Users/asumaran/Developer/woocommerce-gateway-stripe/node_modules/@wordpress/eslint-plugin/index.js)

Now it reports the @wordpress/no-global-event-listener rule is not defined 😃

/Users/asumaran/Developer/woocommerce-gateway-stripe/client/utils/use-confirm-navigation.js
  32:3  error  Definition for rule '@wordpress/no-global-event-listener' was not found  @wordpress/no-global-event-listener
  36:4  error  Definition for rule '@wordpress/no-global-event-listener' was not found  @wordpress/no-global-event-listener

It will also report a lot more eslint issues. Most of them are related to the import/order rule. So not big deal.

The problem with the approach I've used is that if @woocommerce/eslint-plugin updates their @wordpress/eslint-plugin dependency we'll have to update manually the dev dependency to keep them in sync.

Maybe there's a way to consolidate the dependencies on our end or maybe we could update .eslintrc.js to load the correct version of the @wordpress/eslint-plugin package based on the version defined in @woocommerce/eslint-package.

wjrosa · 2024-01-12T21:18:19Z

@wjrosa I've forced eslint to use @wordpress/eslint-plugin@11.1.0 by adding the package as a dev dependency of WCStripe so it takes precedence over the @wordpress/eslint-plugin@8.0.2 by running:
npm install @wordpress/eslint-plugin@^11.0.0 -D
and it loaded the correct rules this time:
- /Users/asumaran/Developer/woocommerce-gateway-stripe/node_modules/@woocommerce/eslint-plugin/index.js
2024-01-12T00:41:29.007Z eslintrc:config-array-factory Loaded: /Users/asumaran/Developer/woocommerce-gateway-stripe/node_modules/@woocommerce/eslint-plugin/configs/custom.js (/Users/asumaran/Developer/woocommerce-gateway-stripe/node_modules/@woocommerce/eslint-plugin/configs/custom.js)
2024-01-12T00:41:29.007Z eslintrc:config-array-factory Loading JS config file: /Users/asumaran/Developer/woocommerce-gateway-stripe/node_modules/@woocommerce/eslint-plugin/configs/custom.js
2024-01-12T00:41:29.007Z eslintrc:config-array-factory Loading plugin "@wordpress" from /Users/asumaran/Developer/woocommerce-gateway-stripe/node_modules/@woocommerce/eslint-plugin/configs/custom.js
2024-01-12T00:41:29.008Z eslintrc:config-array-factory Loaded: @wordpress/eslint-plugin@11.1.0 (/Users/asumaran/Developer/woocommerce-gateway-stripe/node_modules/@wordpress/eslint-plugin/index.js)
Now it reports the @wordpress/no-global-event-listener rule is not defined 😃
/Users/asumaran/Developer/woocommerce-gateway-stripe/client/utils/use-confirm-navigation.js
  32:3  error  Definition for rule '@wordpress/no-global-event-listener' was not found  @wordpress/no-global-event-listener
  36:4  error  Definition for rule '@wordpress/no-global-event-listener' was not found  @wordpress/no-global-event-listener
It will also report a lot more eslint issues. Most of them are related to the import/order rule. So not big deal.

The problem with the approach I've used is that if @woocommerce/eslint-plugin updates their @wordpress/eslint-plugin dependency we'll have to update manually the dev dependency to keep them in sync.

Maybe there's a way to consolidate the dependencies on our end or maybe we could update .eslintrc.js to load the correct version of the @wordpress/eslint-plugin package based on the version defined in @woocommerce/eslint-package.

@asumaran Another option would be to create a specific version for us of @woocommerce/eslint-plugin, but I'm not sure if it is worth it.

asumaran · 2024-01-12T22:59:34Z

@wjrosa I don't know. But I think the way ESLint is currently set up in our repo is not correct. @woocommerce/eslint-plugin@2.2.0 is using old config rules.

wjrosa · 2024-01-13T18:02:31Z

@Mayisha is this something you would have more information about?

Mayisha · 2024-01-15T02:46:17Z

@wjrosa my bad that I did not notice that it's still loading an older version of @wordpress/eslint-plugin instead of 11.0.0 😶‍🌫️
Thanks @asumaran for checking this. 😥

I checked the versions in package-lock.json and discovered that @wordpress/eslint-plugin@8.0.2 is installed as a dependency of @wordpress/scripts (13.0.2). Then finally 8.0.2 is getting installed and loaded instead of 11.0.01.

This might happen because of a few reasons, like the load order of the packages, or the dependency resolution algorithm of npm which determines the versions of packages to install, or 8.0.2 somehow being considered as the compatible version with both @woocommerce/eslint-plugin and @wordpress/scripts, etc.

We can consider a few options here.

Install @wordpress/eslint-plugin@^11.0.0 separately. This works as you have already tried, but introduces some other eslint issues (mostly import/order related)
Update the version of @wordpress/scripts so that the dependency matches to the same version of @wordpress/eslint-plugin@^11.0.0. I noticed that we are using a pretty old version of @wordpress/scripts. However updating it's version is not straightforward as well. It also introduces similar eslint issues (mostly import/order related) and also requires updating our webpack config (npm start does not work right after the version update and gives some error related to webpack config)
Delete node_modules and package-lock.json and then run npm install. This one created a very different package-lock.json for me and I had to add a few @babel/xyz packages as dev dependency. It also introduces some new eslint issues (import/order, spacing etrc)

All of the changes require fixing a number of eslint issues which may introduce a lot of conflict with our deferred intent project if we make the changes now 👀

wjrosa · 2024-01-15T13:51:33Z

@Mayisha, thanks for your input here; it is very valuable! Yeah, it is probably not worth it doing it now. We can get back to this later.

Updating ESLint plugin version

13e0705

wjrosa added the pr: in progress label Jan 4, 2024

wjrosa self-assigned this Jan 4, 2024

Wesley Rosa added 2 commits January 5, 2024 12:11

Add missing packages

d4c2ed8

Ignoring some rules for now

fa0cb00

wjrosa added pr: needs review and removed pr: in progress labels Jan 5, 2024

wjrosa requested review from a team, Mayisha and diegocurbelo and removed request for a team January 5, 2024 17:49

Mayisha approved these changes Jan 11, 2024

View reviewed changes

.eslintrc.js Show resolved Hide resolved

client/components/payment-method-fees-pill/index.js Show resolved Hide resolved

Wesley Rosa added 2 commits January 11, 2024 14:27

Add todo comment to disabled rules in .eslintrc.js

a19546b

Adding recommended rules for @typescript-eslint and disabling some to…

03c277b

… avoid conflicts with upcoming changes

wjrosa merged commit 285bbf7 into develop Jan 11, 2024
32 checks passed

wjrosa deleted the issue/2270-updating-eslint-plugin-version branch January 11, 2024 17:51

wjrosa removed the pr: needs review label Mar 19, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Updating ESLint plugin version #2819

Updating ESLint plugin version #2819

wjrosa commented Jan 4, 2024 •

edited

Mayisha left a comment

asumaran commented Jan 11, 2024

asumaran commented Jan 11, 2024

wjrosa commented Jan 11, 2024

asumaran commented Jan 11, 2024

wjrosa commented Jan 11, 2024

asumaran commented Jan 11, 2024

asumaran commented Jan 12, 2024 •

edited

wjrosa commented Jan 12, 2024

asumaran commented Jan 12, 2024 •

edited

wjrosa commented Jan 13, 2024

Mayisha commented Jan 15, 2024

wjrosa commented Jan 15, 2024

Updating ESLint plugin version #2819

Updating ESLint plugin version #2819

Conversation

wjrosa commented Jan 4, 2024 • edited

Changes proposed in this Pull Request:

Testing instructions

Mayisha left a comment

Choose a reason for hiding this comment

asumaran commented Jan 11, 2024

asumaran commented Jan 11, 2024

wjrosa commented Jan 11, 2024

asumaran commented Jan 11, 2024

wjrosa commented Jan 11, 2024

asumaran commented Jan 11, 2024

asumaran commented Jan 12, 2024 • edited

wjrosa commented Jan 12, 2024

asumaran commented Jan 12, 2024 • edited

wjrosa commented Jan 13, 2024

Mayisha commented Jan 15, 2024

wjrosa commented Jan 15, 2024

wjrosa commented Jan 4, 2024 •

edited

asumaran commented Jan 12, 2024 •

edited

asumaran commented Jan 12, 2024 •

edited