Remove block-editor package usage from components #60999
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
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. |
mirka
reviewed
Apr 23, 2024
There was a problem hiding this comment.
Thank you for debugging this! This probably started around #60793 for me and I thought it was just me.
As for the fix, I actually already removed the wp-block-editor Iframe dependency from that story (#53182) and it looks like I forgot to remove the import. I don't think we should be using wp-block-editor components anywhere in wp-components, including the stories, so I'd prefer removing the now-unused import rather than changing the tsconfig.
If necessary, we can add a lint rule to prevent @wordpress/block-editor imports.
|
When I try to compile this, I get errors (likely preexisting) that I suspect would be fixed by #60796. I suspect ariakit depends on a more recent version of react types than we have: |
|
If the import is unused, it's certainly better to remove it. I'll push that to this PR. I still believe it would valuable to split out the stories from the package build. |
|
@mirka The block editor package is still used here in stories: gutenberg/packages/components/src/utils/hooks/stories/use-cx.js Lines 69 to 90 in 4ec0363 |
|
I've pushed a change that separates out the stories into their own project. |
I'll go ahead and remove this file for now since it's not included in our Storybook, the hook is not publicly exported, we'll likely migrate off of Emotion soon, and the examples seem to be broken at the moment. |
mirka
reviewed
Apr 23, 2024
| @@ -12,8 +12,7 @@ | |||
| ], | |||
| // TODO: Remove `skipLibCheck` after resolving duplicate declaration of the `process` variable | |||
| // between `@types/webpack-env` (from @storybook packages) and `gutenberg-env`. | |||
| "skipLibCheck": true, | |||
| "strictNullChecks": true | |||
There was a problem hiding this comment.
Note for posterity: strictNullChecks was redundant here because it's included in the "strict": true of the base config.
|
Size Change: 0 B Total Size: 1.74 MB ℹ️ View Unchanged
|
Yes 🙂 I think that's ideal, but it may be trickier than isolating the stories. That has the added advantage of isolating the testing environment from the production one like the globals gutenberg/packages/components/tsconfig.json Lines 7 to 12 in a9fbf74 |
|
If we've removed all references to block-editor, I'm fine with reverting the TS project changes and leaving them for follow-up work. What do you think @mirka? |
|
Now when we removed all the However, there's this We are only lucky that it's not a TypeScript file yet. I suspect this code should be in an entirely different package if it needs to read block editor settings. |
sirreal
force-pushed
the
fix/components-stories-types-build
branch
from
830883f to
697026f
Compare
|
I've reverted the tsconfig changes from this PR, now it only includes the removals of |
sirreal
changed the title
|
@WordPress/native-mobile FYI #60999 (comment) |
jsnajdr
approved these changes
Apr 24, 2024
Hey there 👋 I'll work on the issues with |
sirreal
force-pushed
the
fix/components-stories-types-build
branch
from
697026f to
a7b04df
Compare
This has been addressed in #61102 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
Remove references to
@wordpress/block-editorfrom@wordpress/componentspackage.Why?
There is story that references the block-editor project:
gutenberg/packages/components/src/popover/stories/index.story.tsx
Line 11 in 6527034
This crosses TS project boundaries and may trigger errors like this:
This is because the block-editor project and the components project both attempt to write the same files because project references do not reflect this interdependence. In face, block-editor depends on components, which would create a circular dependency between the projects which TypeScript does not allow.
If we'd like to typecheck component stories, it would be better to separate them out into their own TypeScript project.
How?
Excluding all stories removes the problematic undeclared circular dependency and fixes the issue.
Testing Instructions
The following should complete without error: