Page MenuHomePhabricator
Differential D6694

Add ESLint rule to enforce extensions for imports
ClosedPublic
Actions

Authored by ashoat on Feb 11 2023, 12:05 PM.
Tags
None
Referenced Files
Unknown Object (File)
Sat, Nov 2, 10:22 AM
Unknown Object (File)
Oct 29 2024, 2:03 AM
Unknown Object (File)
Oct 28 2024, 11:43 PM
Unknown Object (File)
Oct 27 2024, 5:51 PM
Unknown Object (File)
Oct 12 2024, 3:44 PM
Unknown Object (File)
Oct 8 2024, 9:44 AM
Unknown Object (File)
Sep 28 2024, 10:16 PM
Unknown Object (File)
Sep 28 2024, 10:16 PM
View All Files
Subscribers
michal

Details

Reviewers
atul
tomek
Commits
rCOMM990cc6a81dd3: Add ESLint rule to enforce extensions for imports
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
ashoat added inline comments.Feb 11 2023, 12:43 PM
web/root.js
9–10 ↗(On Diff #22337)

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 #22337)

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

ashoat updated this revision to Diff 22341.Feb 11 2023, 1:33 PM

Rebase

ashoat updated this revision to Diff 22342.Feb 11 2023, 1:33 PM

Rebase

ashoat edited the summary of this revision. (Show Details)Feb 11 2023, 1:35 PM
ashoat added inline comments.Feb 11 2023, 1:42 PM
.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

Harbormaster failed remote builds in B16339: Diff 22341!Feb 11 2023, 1:46 PM
Harbormaster failed remote builds in B16340: Diff 22342!
ashoat updated this revision to Diff 22345.Feb 12 2023, 6:15 AM

Fix accidental duplicate import

Harbormaster completed remote builds in B16343: Diff 22345.Feb 12 2023, 6:29 AM
ashoat updated this revision to Diff 22347.Feb 12 2023, 6:29 AM

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

ashoat added inline comments.Feb 12 2023, 6:43 AM
.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

Harbormaster completed remote builds in B16345: Diff 22347.Feb 12 2023, 6:44 AM
ashoat updated this revision to Diff 22348.Feb 12 2023, 11:33 AM

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

Harbormaster completed remote builds in B16346: Diff 22348.Feb 12 2023, 11:48 AM
ashoat added inline comments.Feb 12 2023, 4:28 PM
.eslintrc.json
39

Here I actually added the rule. Docs here

desktop/src/auto-update.js
3

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

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

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

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

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

keyserver/src/responders/website-responders.js
3

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

8

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

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

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

native/components/swipeable.js
5

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

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

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

native/redux/dev-tools.js
14

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

native/redux/redux-setup.js
517

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

native/root.react.js
19

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

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

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

scripts/get_clang_paths_cli.js
3

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

web/account/siwe-button.react.js
4

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

web/chat/chat-thread-list-see-more-sidebars.react.js
5

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

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

atul accepted this revision.Feb 12 2023, 7:12 PM
This revision is now accepted and ready to land.Feb 12 2023, 7:12 PM
Closed by commit rCOMM990cc6a81dd3: Add ESLint rule to enforce extensions for imports (authored by ashoat). · Explain WhyFeb 12 2023, 7:46 PM
This revision was automatically updated to reflect the committed changes.
ashoat added a commit: rCOMM990cc6a81dd3: Add ESLint rule to enforce extensions for imports.
ashoat mentioned this in D6729: [native] Omit extensions when importing components with platform-specific extensions.Feb 13 2023, 5:13 PM
ashoat mentioned this in rCOMM0e2804a3c8ca: [native] Omit extensions when importing components with platform-specific….Feb 14 2023, 5:16 AM
ashoat mentioned this in D6894: [desktop] Handle macos notifications.Feb 28 2023, 6:12 AM

Revision Contents
Changeset List

PathSize
1 line
desktop/
src/
1 line
1 line
1 line
keyserver/
src/
responders/
3 lines
3 lines
landing/
1 line
lib/
utils/
2 lines
native/
components/
1 line
navigation/
2 lines
push/
2 lines
redux/
2 lines
2 lines
2 lines
types/
13 lines
utils/
2 lines
scripts/
2 lines
web/
account/
2 lines
chat/
2 lines
4 lines
1 line

Diff 22348

View Options

.eslintrc.json

Loading...
View Options

desktop/src/auto-update.js

Loading...
View Options

desktop/src/main.js

Loading...
View Options

desktop/src/preload.js

Loading...
View Options

keyserver/src/responders/landing-handler.js

Loading...
View Options

keyserver/src/responders/website-responders.js

Loading...
View Options

landing/script.js

Loading...
View Options

lib/utils/wagmi-utils.js

Loading...
View Options

native/components/swipeable.js

Loading...
View Options

native/navigation/app-navigator.react.js

Loading...
View Options

native/push/push-handler.react.js

Loading...
View Options

native/redux/dev-tools.js

Loading...
View Options

native/redux/redux-setup.js

Loading...
View Options

native/root.react.js

Loading...
View Options

native/types/react-native.js

Loading...
View Options

native/utils/dev-hostname.js

Loading...
View Options

scripts/get_clang_paths_cli.js

Loading...
View Options

web/account/siwe-button.react.js

Loading...
View Options

web/chat/chat-thread-list-see-more-sidebars.react.js

Loading...
View Options

web/root.js

Loading...
View Options

web/script.js

Loading...