-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Add ESLint import resolver #31792
Add ESLint import resolver #31792
Conversation
Size Change: 0 B Total Size: 1.3 MB ℹ️ View Unchanged
|
.github/workflows/static-checks.yml
Outdated
- name: Type checking | ||
run: npm run build:package-types | ||
- name: Build and type checking | ||
run: npm run build |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We probably don't even need to run the full build in this workflow? Just build:package-types
is enough?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed the build step. We can add it back if we really need it.
Great idea to use the custom resolver to remove the need for the build step for Gutenberg. We do something similar already for unit tests with a Jest mapper 👍🏻
I don't think we need a package. It isn't applicable outside of Gutenberg. It will make this PR simpler. |
Sure, but where should I put this file then? I don't think it belongs in |
There is the
|
e7ee3ff
to
60eae9c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, thank you for working on this improvement for CI.
I left two minor comments to address before merging.
@youknowriad noticed that WordPress dependencies can be added to the code and ESLint doesn't report it as a violation. It was caught during backporting to the
Those violations were already fixed in the The simplest way to test is to disable the resolver and import anything from |
Good catch! I indeed missed this case, I'll see what I can do in a follow-up PR. |
Description
eslint-plugin-import
is using the default import resolver to resolve the packages. By default, it's looking for the "main" field inpackage.json
. This works for most packages, but not automatically for internal@wordpress/*
packages. We need to at least runnpm run dev
ornpm run build
once to generate the build files in order for the resolver to work. It isn't a big deal though, but wouldn't it be better if we can skip this step and just automatically reference the file to their source?This PR introduces a new internal package
@wordpress/eslint-import-resolver
to handle the resolution of the@wordpress/*
packages. It tries to fallback to finding the source file insrc/index.js
whenever the "main" file isn't present. For other external dependencies, it simply fallbacks to the default resolution usingeslint-import-resolver-node
.The benefit of this is that it no longer requires the developers to build the packages before they can run
lint-js
, or before the code editor's built-in linter shout at them.How has this been tested?
Return to a "clean state" of the project, that is, remove all of the
build
folders.Install the dependencies, and then
lint-js
script should work without extra steps. Or, do a clean clone of the project to a new directory.Types of changes
New feature
Checklist:
*.native.js
files for terms that need renaming or removal).