Testing: Remove custom import resolvers and package subpath syntax rules#72978
Testing: Remove custom import resolvers and package subpath syntax rules#72978
Conversation
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
|
Evidenced by the build, one downside of this is that it requires an upfront |
|
Size Change: 0 B Total Size: 2.58 MB ℹ️ View Unchanged
|
|
Flaky tests detected in 623f8a2. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/19838573135
|
Possible solutions:
|
ee33892 to
a6ff3de
Compare
In a6ff3de, I ended up with a hybrid approach reintroducing the custom resolver, but enhancing it with support to consider subpaths using the package's |
a6ff3de to
9dc01ec
Compare
|
What would be the impact on DevX if we required build before linting? Is it just for CI? If that's the case maybe we should just go for it? |
I think it could be negative enough in regular development workflows to be worth trying to avoid. It might not always crop up because usually a regular contributor will likely have some form of built packages in their local copy of the project, but (a) it could be a barrier for new contributors working from a fresh clone and (b) could create issues where linting identifies issues only because the built copy is out of date. In both cases, these would clear up after running |
| wp: 'readonly', | ||
| }, | ||
| settings: { | ||
| 'import/internal-regex': wpPackagesRegExp, |
There was a problem hiding this comment.
Noting that this can have an impact on rules like import/order in considering packages as "internal", though (a) this base config exists in a reusable package for third-party consumption where @wordpress/ packages are not internal, and (b) even inside the context of this project, we distinguish @wordpress/ packages from "internal" dependencies.
It might still make sense to define this at the root /.eslintrc.js since, after all, the setting is described as "useful when you are utilizing a monorepo setup or developing a set of packages that depend on each other". But it doesn't seem like something which will actually have much of an impact in practical usage from what I can tell.
Related: #73396
There was a problem hiding this comment.
Noting that this can have an impact on rules like
import/orderin considering packages as "internal", though (a) this base config exists in a reusable package for third-party consumption where@wordpress/packages are not internal, and (b) even inside the context of this project, we distinguish@wordpress/packages from "internal" dependencies.
I added an extra CHANGELOG note clarifying this in 0748af8.
It might still make sense to define this at the root
/.eslintrc.jssince, after all, the setting is described as "useful when you are utilizing a monorepo setup or developing a set of packages that depend on each other". But it doesn't seem like something which will actually have much of an impact in practical usage from what I can tell.
Funny enough, while this might make sense to do, I recalled that one of the changes I'm making in this branch is to remove the code where we explicitly unset this value in our .eslintrc.js. So this should change should have no net effect in this project. Ironic that we were defining this default internal regular expression and not using it in the one place it might make sense to do so 😅
|
I had some trouble with the tests for
|
tools/eslint/import-resolver.js
Outdated
| // source file using its declared exports. | ||
| try { | ||
| const manifestPath = path.join( packagePath, 'package.json' ); | ||
| const manifest = require( manifestPath ); |
There was a problem hiding this comment.
If you know the exact path to the package.json, and don't need to resolve it, it's better to read and parse the JSON file "manually". require is supposed to respect the exports field, and should fail when package.json is not declared as export. If it doesn't fail now, it will eventually start failing one day 🙂
If we ever declare package.json as a package export, it's always because of slightly broken tools like this.
There was a problem hiding this comment.
If you know the exact path to the
package.json, and don't need to resolve it, it's better to read and parse the JSON file "manually".requireis supposed to respect theexportsfield, and should fail whenpackage.jsonis not declared as export. If it doesn't fail now, it will eventually start failing one day 🙂If we ever declare
package.jsonas a package export, it's always because of slightly broken tools like this.
That's a good call. I've updated in aa8ab95
| } from '../fixtures'; | ||
|
|
||
| /* eslint-disable no-restricted-syntax */ | ||
| import * as form from '@wordpress/block-library/src/form'; |
There was a problem hiding this comment.
This is interesting. The src folder is not a part of block-library's exports field, so this import should fail.
It turns out that block-library doesn't export the individual blocks at all, the src/index.js file doesn't re-export them. How can we fix this?
This question is also very relevant for #73516, where the entire block-library is going to be bundled into one block-library/build-module/index.js file and the individual block files and folders don't exist any more.
There was a problem hiding this comment.
This is interesting. The
srcfolder is not a part ofblock-library'sexportsfield, so this import should fail.
Yeah, that's a good point.
From what I can tell:
- The import lint rule doesn't fail because we disable
import/no-unresolvedfor "development" files. I'm not sure why we do that. It seems to stem back to Code quality: Enable import/no-unresolved ESLint rule for Gutenberg #20905 where we may have been incrementally restricting files, but never got around to enforcing it fully. - My guess as to why the test code itself doesn't fail is that Jest is not fully respecting entrypoints? There's another simpler example in
block-serialization-default-parser
My impression was that these block implementations were meant to be exposed through the build-module wildcard entrypoint. We could probably update this code to use those paths, but as you mention, that becomes incompatible with your changes in #73516.
Some ideas for how we could move forward there:
- Could we specially handle wildcard entrypoints to preserve the existing behavior in just those instances?
- Maybe we don't actually want to compile these down to a single file? 😬
- We could add individual
exportsfor each and every block, assuming your changes support that.
All that being said, I don't think any of this is necessarily a blocker for the changes I'm proposing here.
There was a problem hiding this comment.
My impression was that these block implementations were meant to be exposed through the
build-modulewildcard entrypoint.
Yes that's right, even the docs explicitly mention the build-module imports:
It would be nicer if instead of importing @wordpress/block-library/build-module/verse/init.js we could import just @wordpress/block-library/verse/init.js, but that would probably require an exports entry for each of the 100+ Core blocks.
We could probably update this code to use those paths
When I try this locally, it works. In the full-content.test.js file, replace the /src/ imports with /build-module/. And also update the src/*/block.json glob. Then run npm run fixtures:generate. It will regenerate the fixtures in test/integration/fixtures/blocks, even if you delete the *.json and *.serialized.html files there.
Let's do this as part of this PR, as we're touching the file anyway.
Maybe we don't actually want to compile these down to a single file?
Yes, the block-library package is special. The single-file bundle still makes sense, but additionally we'll want to expose each block individually. The block.json file and the edit/save/view/init scripts.
There is prior art for this. Ariakit bundles its packages, and @ariakit/react has one big bundle with all components, and also supports individual exports like @ariakit/react/button.
There was a problem hiding this comment.
It would be nicer if instead of importing
@wordpress/block-library/build-module/verse/init.jswe could import just@wordpress/block-library/verse/init.js, but that would probably require anexportsentry for each of the 100+ Core blocks.
AFAIK it's perfectly valid for us to do something like this, which would achieve that effect?
"exports": {
"./*", "./build-module/*"
}
There was a problem hiding this comment.
AFAIK it's perfectly valid for us to do something like this, which would achieve that effect?
I'll try this in #73516, which needs a major fix for block-library anyway 🙂
The only possible risk is that the build-module folder will contain something that we don't want to export. But especially with the bundling done in #73516 this shouldn't be happening.
jsnajdr
left a comment
There was a problem hiding this comment.
Looks good. But please consider updating the block-library/src imports in full-content.test.js, it should be a small change.
|
I hesitated on changing the imports to use I went a different direction in #73705, opting to remove these imports altogether. |
623f8a2 to
4847774
Compare
Redundant with improved resolver behavior
Redundant with TypeScript resolver changes
This handles the Babel stuff. It should probably live in eslint-plugin
There are a few early returns or thrown (suppressed) errors possible before when subpath is actually used, so avoid wasting effort if it's not going to be used
Avoid potential incompatibility if `require` enforces entrypoints
4847774 to
1f08cb1
Compare
`@wordpress` modules are no longer excluded from the `import/no-extraneous-dependencies` rule. See: WordPress/gutenberg#72978
Address lint errors following recent changes to the `@wordpress/eslint-plugin` package. Align with the Gutenberg project. See: WordPress/gutenberg#72978 ``` ./GutenbergKit/src/utils/wordpress-globals.js 47:41 error Unable to resolve path to module '@wordpress/preferences-persistence' import/no-unresolved ```
…#286) * build(deps-dev): Bump @wordpress/eslint-plugin from 22.22.0 to 24.0.0 Bumps [@wordpress/eslint-plugin](https://github.com/WordPress/gutenberg/tree/HEAD/packages/eslint-plugin) from 22.22.0 to 24.0.0. - [Release notes](https://github.com/WordPress/gutenberg/releases) - [Changelog](https://github.com/WordPress/gutenberg/blob/trunk/packages/eslint-plugin/CHANGELOG.md) - [Commits](https://github.com/WordPress/gutenberg/commits/@wordpress/eslint-plugin@24.0.0/packages/eslint-plugin) --- updated-dependencies: - dependency-name: "@wordpress/eslint-plugin" dependency-version: 24.0.0 dependency-type: direct:development update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <support@github.com> * style: Address lint errors from undeclared WordPress dependencies `@wordpress` modules are no longer excluded from the `import/no-extraneous-dependencies` rule. See: WordPress/gutenberg#72978 * build: Align ESLint import resolver with WordPress packages Address lint errors following recent changes to the `@wordpress/eslint-plugin` package. Align with the Gutenberg project. See: WordPress/gutenberg#72978 ``` ./GutenbergKit/src/utils/wordpress-globals.js 47:41 error Unable to resolve path to module '@wordpress/preferences-persistence' import/no-unresolved ``` * task: Regenerate `@wordpress/rich-text` patch file Regeneration is required after upgrading package versions. ``` **ERROR** Failed to apply patch for package @wordpress/rich-text at path node_modules/@wordpress/rich-text This error was caused because @wordpress/rich-text has changed since you made the patch file for it. This introduced conflicts with your patch, just like a merge conflict in Git when separate incompatible changes are made to the same piece of code. Maybe this means your patch file is no longer necessary, in which case hooray! Just delete it! Otherwise, you need to generate a new patch file. To generate a new one, just repeat the steps you made to generate the first one. i.e. manually make the appropriate file changes, then run patch-package @wordpress/rich-text Info: Patch file: patches/@WordPress+rich-text+7.37.0.patch Patch was made for version: 7.37.0 Installed version: 7.38.0 ``` --------- Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: David Calhoun <github@davidcalhoun.me>
What?
Updates lint configuration to remove redundant import resolvers and syntax rules.
Follow-up to #72893
Why?
These configurations were previously necessary because:
But these are no longer necessary as of:
exports)exports, which is the standard way to define a package's public interface and disallow subpath importsFurthermore, this change will also allow packages to define their own
exportson subpaths without triggering the "Path access on WordPress dependencies is not allowed" lint error.How?
no-restricted-syntaxselector for path access on packagestools/eslint/import-resolver.jscustom resolver and configured references to itTesting Instructions
npm run lint:jsshould pass.Additionally, you can verify that the original intent of the subpath rules is upheld when trying to import from a subpath of a package: