-
Notifications
You must be signed in to change notification settings - Fork 4.6k
UI: Ensure Stack is exported from root package exports #73928
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
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. |
|
Flaky tests detected in bf8e6d5. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/20172074697
|
| __dangerousOptInToUnstableAPIsOnlyForCoreModules( | ||
| 'I acknowledge private features are not for use in themes or plugins and doing so will break in the next version of WordPress.', | ||
| '@wordpress/dataviews' | ||
| ); |
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 can also remove references to @wordpress/private-apis from package.json and tsconfig.json if we don't use it any more.
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.
Good catch. Fixed in b93be57.
packages/ui/src/test/index.test.ts
Outdated
| it( 'should export something from each component', async () => { | ||
| // As described in the CONTRIBUTING.md file, each component should be | ||
| // exported from an index.ts in its implementation directory. | ||
| const components = await glob( join( __dirname, '../*/index.ts' ) ); |
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'm paranoid about breaking in Windows again, could this need this kind of workaround?
For example I'm seeing a normalizer used here:
gutenberg/packages/wp-build/src/build.mjs
Lines 456 to 457 in b59a245
| const cssFiles = await glob( | |
| normalizePath( path.join( buildStyleDir, '**/*.css' ) ) |
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 feel frustrated at the ambiguity that my answer to your question is "I don't know, and I can't easily confirm" 😕
For what it's worth, we recently enabled Windows checks on pull requests in #73923, though I need to rebase the branch for that to apply here, and it only covers "Static checks" (including npm run build), but not the unit tests, so it wouldn't help here.
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.
but not the unit tests, so it wouldn't help here.
Although to that point, it doesn't appear that we run unit tests in a Windows environment at any point, so we have no guarantees that the tests currently work on Windows machines 😬
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've been dabbling a bit with using GitHub Codespaces and this seems like it'd be a great use for testing how code behaves on other systems, but sadly it only offers Linux containers at the moment.
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.
Okay, I understand better now why we have normalizePath, and the short answer to your question is "yes".
It's a requirement of fast-glob:
⚠️ Always use forward-slashes in glob expressions (patterns and ignore option). Use backslashes for escaping characters.
I think we won't need these workarounds once we upgrade to a version of Node.js that includes fs.glob, which I suspect will handle this better (available in Node.js v22+, see #72973).
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.
Fixed in 3bba4f5
fast-glob requires forward slashes. path.join produces backslashes in Windows environment. Simple fix is to avoid path.join
|
Size Change: 0 B Total Size: 2.58 MB ℹ️ View Unchanged
|
Co-authored-by: Lena Morita <[email protected]>
What?
Ensures that the
Stackcomponent is exported from the@wordpress/uipackage.Follow-up to #73308
Why?
At the moment, the component cannot be used from the package.
How?
packages/ui/src/index.tsCHANGELOG.mdlock-unlock.tsfileTesting Instructions
import { Stack } from '@wordpress/ui';code snippet in some code file, and a corresponding"@wordpress/ui": "../ui"dependencies declaration in that code's closestpackage.jsonnpm run lint:jsScreenshots or screencast