Page MenuHomePhabricator

Add ESLint rule to enforce extensions for imports
ClosedPublic

Authored by ashoat on Feb 11 2023, 12:05 PM.
Tags
None
Referenced Files
Unknown Object (File)
Fri, Apr 5, 2:42 PM
Unknown Object (File)
Fri, Apr 5, 2:34 PM
Unknown Object (File)
Mar 28 2024, 9:39 PM
Unknown Object (File)
Mar 28 2024, 9:38 PM
Unknown Object (File)
Mar 28 2024, 9:37 PM
Unknown Object (File)
Mar 28 2024, 9:36 PM
Unknown Object (File)
Mar 28 2024, 9:36 PM
Unknown Object (File)
Mar 25 2024, 1:58 PM
Subscribers

Details

Summary

In D6692, I updated all imports to include .js extensions to satisfy Webpack 5 and Node.js 19. To make sure we don't regress on this, in this diff I introduce an ESLint rule for enforcing extensions for all imports.

To make this diff easier to review, I'll highlight relevant changes with inline comments.

As part of this, the ESLint rule identified some places that the codemod script in D6692 didn't catch. They include:

  1. Places where we use CommonJS require instead of import
  2. Imports from within packages. In D6692, I updated places that just needed a .js extension (eg. lodash/fp/merge.js). But in some cases it was more complicated... I either couldn't find a file at all (eg. electron/main), or found that there was a package.json inside the directory instead of an index.js. I ended up adding eslint-disable macros to hide the cases that were hard to update, or following the package.json indirection to find the real file in the easier cases
    • Note that I could've configured the ESLint rule with ignorePackages instead of always, but I opted not to do this after testing it against my Webpack 5 branch and finding that Webpack wanted imports from within packages to be fully qualifed

Depends on D6693

Test Plan
  1. I ran web, landing, keyserver, and native all together and made sure everything still worked
  2. I tested web against my Webpack 5 branch (coming soon) and noted if any errors appeared or went away regarding places where imports were changed. Usually errors went away, but in some cases I had to test a couple times to make sure the imports worked correctly
  3. In places where I actually changed something, my inline comments sometimes mention ways I tested them

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
web/root.js
9–10

Here I mapped a custom package.json nested inside a package to the file that they would resolve to

web/script.js
7

Here I added an eslint-disable. When I tried to map this to a file, Webpack 5 became unhappy that the file was not specified in exports in react-dom's package.json. I tried Googling but couldn't find a quick solution. Ultimately the unqualified import works fine with Webpack 5 so we're keeping that

.eslintrc.json
39 ↗(On Diff #22342)

Here I actually added the rule. Docs here

desktop/src/auto-update.js
3 ↗(On Diff #22342)

Here I added an eslint-disable. I could not find a JS file this corresponds to, and it is probably some Electron magic that cannot be changed.

I opted to just ignore the ESLint rule, as desktop is not impacted by either the Node.js 19 or Webpack 5 migrations.

cc @michal

desktop/src/main.js
11 ↗(On Diff #22342)

Here I added an eslint-disable. I could not find a JS file this corresponds to, and it is probably some Electron magic that cannot be changed.

I opted to just ignore the ESLint rule, as desktop is not impacted by either the Node.js 19 or Webpack 5 migrations.

desktop/src/preload.js
3 ↗(On Diff #22342)

Here I added an eslint-disable. I could not find a JS file this corresponds to, and it is probably some Electron magic that cannot be changed.

I opted to just ignore the ESLint rule, as desktop is not impacted by either the Node.js 19 or Webpack 5 migrations.

keyserver/src/responders/landing-handler.js
3 ↗(On Diff #22342)

Here I mapped a folder to the index.js file within

7 ↗(On Diff #22342)

Here I added an eslint-disable. When I tried to map this to a file, Webpack 5 became unhappy that the file was not specified in exports in react-dom's package.json. I tried Googling but couldn't find a quick solution. Ultimately the unqualified import works fine with Webpack 5 so we're keeping that

keyserver/src/responders/website-responders.js
3 ↗(On Diff #22342)

Here I mapped a folder to the index.js file within

8 ↗(On Diff #22342)

Here I added an eslint-disable. When I tried to map this to a file, Webpack 5 became unhappy that the file was not specified in exports in react-dom's package.json. I tried Googling but couldn't find a quick solution. Ultimately the unqualified import works fine with Webpack 5 so we're keeping that

landing/script.js
9 ↗(On Diff #22342)

Here I added an eslint-disable. When I tried to map this to a file, Webpack 5 became unhappy that the file was not specified in exports in react-dom's package.json. I tried Googling but couldn't find a quick solution. Ultimately the unqualified import works fine with Webpack 5 so we're keeping that

lib/utils/wagmi-utils.js
4–5 ↗(On Diff #22342)

Here I mapped a custom package.json nested inside a package to the file that they would resolve to

native/components/swipeable.js
5 ↗(On Diff #22342)

This mapped to a custom package.json nested inside a package. It resolves to a .tsx file, but when I tried to import that directly, Metro became unhappy. I opted to just ignore the ESLint rule, as native is not impacted by either the Node.js 19 or Webpack 5 migrations

native/navigation/app-navigator.react.js
6 ↗(On Diff #22342)

Here I mapped a custom package.json nested inside a package to the file that they would resolve to

native/push/push-handler.react.js
49 ↗(On Diff #22342)

I am honestly not sure why this was missed in the script from D6692

native/redux/dev-tools.js
14 ↗(On Diff #22342)

Here I mapped a folder to the index.js file within

native/redux/redux-setup.js
517 ↗(On Diff #22342)

Here I mapped a folder to the index.js file within

native/root.react.js
19 ↗(On Diff #22342)

Here I mapped a custom package.json nested inside a package to the file that they would resolve to

native/types/react-native.js
10–23 ↗(On Diff #22342)

My script in D6692 missed these because they are import statements, not export statements. The blog post that inspired my codemod script warned that this would happen!

native/utils/dev-hostname.js
25 ↗(On Diff #22342)

This was missed in D6692 because it is a require statement, not an import

scripts/get_clang_paths_cli.js
3 ↗(On Diff #22342)

This was missed in D6692 because it is a require statement, not an import

web/account/siwe-button.react.js
4 ↗(On Diff #22342)

Here I mapped a folder to the index.js file within

web/chat/chat-input-bar.react.js
35 ↗(On Diff #22342)

Oops, this is duplicated now on line 40. Will remove the duplicate before landing

web/chat/chat-thread-list-see-more-sidebars.react.js
5 ↗(On Diff #22342)

Here I mapped a folder to the index.js file within

web/root.js
9–10 ↗(On Diff #22342)

Here I mapped a custom package.json nested inside a package to the file that they would resolve to

web/script.js
7 ↗(On Diff #22342)

Here I added an eslint-disable. When I tried to map this to a file, Webpack 5 became unhappy that the file was not specified in exports in react-dom's package.json. I tried Googling but couldn't find a quick solution. Ultimately the unqualified import works fine with Webpack 5 so we're keeping that

Fix accidental duplicate import

Remove accidental ordering change in web/chat/chat-input-bar.react.js

.eslintrc.json
39 ↗(On Diff #22347)

Here I actually added the rule. Docs here

desktop/src/auto-update.js
3 ↗(On Diff #22347)

Here I added an eslint-disable. I could not find a JS file this corresponds to, and it is probably some Electron magic that cannot be changed.

I opted to just ignore the ESLint rule, as desktop is not impacted by either the Node.js 19 or Webpack 5 migrations.

cc @michal

desktop/src/main.js
11 ↗(On Diff #22347)

Here I added an eslint-disable. I could not find a JS file this corresponds to, and it is probably some Electron magic that cannot be changed.

I opted to just ignore the ESLint rule, as desktop is not impacted by either the Node.js 19 or Webpack 5 migrations.

desktop/src/preload.js
3 ↗(On Diff #22347)

Here I added an eslint-disable. I could not find a JS file this corresponds to, and it is probably some Electron magic that cannot be changed.

I opted to just ignore the ESLint rule, as desktop is not impacted by either the Node.js 19 or Webpack 5 migrations.

keyserver/src/responders/landing-handler.js
3 ↗(On Diff #22347)

Here I mapped a folder to the index.js file within

7 ↗(On Diff #22347)

Here I added an eslint-disable. When I tried to map this to a file, Webpack 5 became unhappy that the file was not specified in exports in react-dom's package.json. I tried Googling but couldn't find a quick solution. Ultimately the unqualified import works fine with Webpack 5 so we're keeping that

keyserver/src/responders/website-responders.js
3 ↗(On Diff #22347)

Here I mapped a folder to the index.js file within

8 ↗(On Diff #22347)

Here I added an eslint-disable. When I tried to map this to a file, Webpack 5 became unhappy that the file was not specified in exports in react-dom's package.json. I tried Googling but couldn't find a quick solution. Ultimately the unqualified import works fine with Webpack 5 so we're keeping that

landing/script.js
9 ↗(On Diff #22347)

Here I added an eslint-disable. When I tried to map this to a file, Webpack 5 became unhappy that the file was not specified in exports in react-dom's package.json. I tried Googling but couldn't find a quick solution. Ultimately the unqualified import works fine with Webpack 5 so we're keeping that

lib/utils/wagmi-utils.js
4–5 ↗(On Diff #22347)

Here I mapped a custom package.json nested inside a package to the file that they would resolve to

native/components/swipeable.js
5 ↗(On Diff #22347)

This mapped to a custom package.json nested inside a package. It resolves to a .tsx file, but when I tried to import that directly, Metro became unhappy. I opted to just ignore the ESLint rule, as native is not impacted by either the Node.js 19 or Webpack 5 migrations

native/navigation/app-navigator.react.js
6 ↗(On Diff #22347)

Here I mapped a custom package.json nested inside a package to the file that they would resolve to

native/push/push-handler.react.js
49 ↗(On Diff #22347)

I am honestly not sure why this was missed in the script from D6692

native/redux/dev-tools.js
14 ↗(On Diff #22347)

Here I mapped a folder to the index.js file within

native/redux/redux-setup.js
517 ↗(On Diff #22347)

Here I mapped a folder to the index.js file within

native/root.react.js
19 ↗(On Diff #22347)

Here I mapped a custom package.json nested inside a package to the file that they would resolve to

native/types/react-native.js
10–23 ↗(On Diff #22347)

My script in D6692 missed these because they are import statements, not export statements. The blog post that inspired my codemod script warned that this would happen!

native/utils/dev-hostname.js
25 ↗(On Diff #22347)

This was missed in D6692 because it is a require statement, not an import

scripts/get_clang_paths_cli.js
3 ↗(On Diff #22347)

This was missed in D6692 because it is a require statement, not an import

web/account/siwe-button.react.js
4 ↗(On Diff #22347)

Here I mapped a folder to the index.js file within

web/chat/chat-thread-list-see-more-sidebars.react.js
5 ↗(On Diff #22347)

Here I mapped a folder to the index.js file within

web/root.js
9–10 ↗(On Diff #22347)

Here I mapped a custom package.json nested inside a package to the file that they would resolve to

web/script.js
7 ↗(On Diff #22347)

Here I added an eslint-disable. When I tried to map this to a file, Webpack 5 became unhappy that the file was not specified in exports in react-dom's package.json. I tried Googling but couldn't find a quick solution. Ultimately the unqualified import works fine with Webpack 5 so we're keeping that

Webpack 5 is actually okay with importing from within Wagmi since Wagmi's package.json declares exports. Not so for redux-persist, on the other hand

.eslintrc.json
39 ↗(On Diff #22348)

Here I actually added the rule. Docs here

desktop/src/auto-update.js
3 ↗(On Diff #22348)

Here I added an eslint-disable. I could not find a JS file this corresponds to, and it is probably some Electron magic that cannot be changed.

I opted to just ignore the ESLint rule, as desktop is not impacted by either the Node.js 19 or Webpack 5 migrations.

cc @michal

desktop/src/main.js
11 ↗(On Diff #22348)

Here I added an eslint-disable. I could not find a JS file this corresponds to, and it is probably some Electron magic that cannot be changed.

I opted to just ignore the ESLint rule, as desktop is not impacted by either the Node.js 19 or Webpack 5 migrations.

desktop/src/preload.js
3 ↗(On Diff #22348)

Here I added an eslint-disable. I could not find a JS file this corresponds to, and it is probably some Electron magic that cannot be changed.

I opted to just ignore the ESLint rule, as desktop is not impacted by either the Node.js 19 or Webpack 5 migrations.

keyserver/src/responders/landing-handler.js
3 ↗(On Diff #22348)

Here I mapped a folder to the index.js file within

7 ↗(On Diff #22348)

Here I added an eslint-disable. When I tried to map this to a file, Webpack 5 became unhappy that the file was not specified in exports in react-dom's package.json. I tried Googling but couldn't find a quick solution. Ultimately the unqualified import works fine with Webpack 5 so we're keeping that

keyserver/src/responders/website-responders.js
3 ↗(On Diff #22348)

Here I mapped a folder to the index.js file within

8 ↗(On Diff #22348)

Here I added an eslint-disable. When I tried to map this to a file, Webpack 5 became unhappy that the file was not specified in exports in react-dom's package.json. I tried Googling but couldn't find a quick solution. Ultimately the unqualified import works fine with Webpack 5 so we're keeping that

landing/script.js
9 ↗(On Diff #22348)

Here I added an eslint-disable. When I tried to map this to a file, Webpack 5 became unhappy that the file was not specified in exports in react-dom's package.json. I tried Googling but couldn't find a quick solution. Ultimately the unqualified import works fine with Webpack 5 so we're keeping that

lib/utils/wagmi-utils.js
4–7 ↗(On Diff #22348)

wagmi declares these as exports on its package.json, so Webpack is okay with it

native/components/swipeable.js
5 ↗(On Diff #22348)

This mapped to a custom package.json nested inside a package. It resolves to a .tsx file, but when I tried to import that directly, Metro became unhappy. I opted to just ignore the ESLint rule, as native is not impacted by either the Node.js 19 or Webpack 5 migrations

native/navigation/app-navigator.react.js
6 ↗(On Diff #22348)

Here I mapped a custom package.json nested inside a package to the file that they would resolve to

native/push/push-handler.react.js
49 ↗(On Diff #22348)

I am honestly not sure why this was missed in the script from D6692

native/redux/dev-tools.js
14 ↗(On Diff #22348)

Here I mapped a folder to the index.js file within

native/redux/redux-setup.js
517 ↗(On Diff #22348)

Here I mapped a folder to the index.js file within

native/root.react.js
19 ↗(On Diff #22348)

Here I mapped a custom package.json nested inside a package to the file that they would resolve to

native/types/react-native.js
10–23 ↗(On Diff #22348)

My script in D6692 missed these because they are import statements, not export statements. The blog post that inspired my codemod script warned that this would happen!

native/utils/dev-hostname.js
25 ↗(On Diff #22348)

This was missed in D6692 because it is a require statement, not an import

scripts/get_clang_paths_cli.js
3 ↗(On Diff #22348)

This was missed in D6692 because it is a require statement, not an import

web/account/siwe-button.react.js
4 ↗(On Diff #22348)

Here I mapped a folder to the index.js file within

web/chat/chat-thread-list-see-more-sidebars.react.js
5 ↗(On Diff #22348)

Here I mapped a folder to the index.js file within

web/root.js
9–10 ↗(On Diff #22348)

Here I mapped a custom package.json nested inside a package to the file that they would resolve to

web/script.js
7 ↗(On Diff #22348)

Here I added an eslint-disable. When I tried to map this to a file, Webpack 5 became unhappy that the file was not specified in exports in react-dom's package.json. I tried Googling but couldn't find a quick solution. Ultimately the unqualified import works fine with Webpack 5 so we're keeping that

This revision is now accepted and ready to land.Feb 12 2023, 7:12 PM