wordpress-seo
wordpress-seo copied to clipboard
Upgrade UI library dependencies
Context
- Upgrade Storybook
- ⚠️ This upgrade breaks the Storyshots, they now always output empty snapshots. However, this does not alter the always passing that they (apparently) already did. In Storybook 8, Storyshots is deprecated. So it makes sense to move away from them too. I tried to get the test-runner to work, but failed. Leaving it for the future.
- Upgrade React
- and moving away from @wordpress/element in favor of direct React. It does not have to be tied to WP and Storybook had some problems with the forwardRef from WP element (with this the StoryComponent exports are no longer needed)
- Replace the @yoast/babel-preset with inline configuration -- as that preset is very WP orientated
Summary
This PR can be summarized in the following changelog entry:
- [@yoast/ui-library 1.0.0] Replaces
@wordpress/element
withReact
18.2.0. - [@yoast/ui-library 0.1.0] Upgrades
@storybook/*
to 7.6.10. - [@yoast/ui-library 0.1.0] Adds browserslist config (using config plugin
@yoast/browserslist-config
). - [@yoast/ui-library 0.1.0] Replaces Babel configuration and development dependencies. Moving away from
@yoast/babel-preset
in favor of@bable/present-env
and@babel/preset-react
. - [@yoast/ui-library 0.1.0] Removes unneeded StoryComponents, as forwardRefs from React are compatible with Storybook.
- [@yoast/ui-library 0.1.0] Upgrades Babel.
- [@yoast/ui-library 0.1.0] Improves documentation of hooks.
- [@yoast/ui-library 0.0.1] Fixes a bug where the
Textarea
element did not use thecols
default.
Relevant technical choices:
- Added the browserslist config because I saw it was missing -- though Dennis made a good point about that possibly being browser focussed and us not having to care for a separate package? So thoughts welcome.
- Replaced the babel config because it is WP specific
- Replaced WP element with React because it is WP specific and I knew it caused an issue with forwardRef in our stories (needing the StoryComponents)
- Ran the Storybook migration script (
npx sb upgrade
) and followed their migration guide. See commit messages for more details - Tried to fix all the errors and warnings that popped up. I think only one remains: when switching between stories there is a warning (or just when visiting a story):
Warning: The final argument passed to useMemo changed size between renders. The order and size of this array must remain constant.
Previous: [addon-controls, addon-controls, storybook/actions/panel, storybook/a11y/panel] Incoming: [addon-controls, addon-controls, storybook/a11y/panel]
- Added
process.env.NODE_DEBUG
to the JS side via Webpack' DefinePlugin. I must admit I don't know why this is happening. This happened after my rebase on trunk and fixing theyarn.lock
files. Below is one of the error message you would get (after a build without any errors) without defining NODE_DEBUG.
Uncaught ReferenceError: process is not defined at ./node_modules/util/util.js (analysis.js?ver=40ecf9fb9b76ee0e83a7:53087:1) at webpack_require (analysis.js?ver=40ecf9fb9b76ee0e83a7:54193:42) at ./packages/yoastseo/src/scoring/seoAssessor.js (analysis.js?ver=40ecf9fb9b76ee0e83a7:35556:62) at webpack_require (analysis.js?ver=40ecf9fb9b76ee0e83a7:54193:42) at ./packages/yoastseo/src/worker/AnalysisWebWorker.js (analysis.js?ver=40ecf9fb9b76ee0e83a7:38676:78) at webpack_require (analysis.js?ver=40ecf9fb9b76ee0e83a7:54193:42) at ./packages/yoastseo/src/worker/index.js (analysis.js?ver=40ecf9fb9b76ee0e83a7:40521:76) at webpack_require (analysis.js?ver=40ecf9fb9b76ee0e83a7:54193:42) at analysis.js?ver=40ecf9fb9b76ee0e83a7:54299:69 at analysis.js?ver=40ecf9fb9b76ee0e83a7:54372:3
- Regarding the ESLint ignore commit: this seemed the easiest solution
- React takes care of controlled vs uncontrolled: value vs defaultValue
- If we provide props for either, we'd have to take over/juggle them the same way
- Storyshots: due to a missing snapshot in
trunk
I noticed that creating snapshots now always results in empty snapshot images. Even though the test result is always a success, regardless of current snapshots not being empty. This was already a problem (we have an issue), I just didn't realize it was this bad. As stated in the context, I tried migrating to test-runner, but failed. So I'm leaving this to the future.
Test instructions
Test instructions for the acceptance test before the PR gets merged
This PR can be acceptance tested by following these steps:
UI library: main part
- Verify the UI library builds
- Verify the UI library components function as before
- Verify the storybook builds and functions
- Note that the docs work a bit differently here. We now have one Docs entry alongside the different stories. And the entries under that are no longer pointing to the stories, but rather (what was) the canvas.
- Before:
- After:
- Before:
- Note that the docs work a bit differently here. We now have one Docs entry alongside the different stories. And the entries under that are no longer pointing to the stories, but rather (what was) the canvas.
Regression due to monorepo shenanigans
I had to fix one error in the webpack build for the plugin, unrelated to the UI library. I suggest a slightly more thorough smoke test.
- Verify our block editor integration still works
- Verify our classic editor integration still works
- Verify our Elementor integration still works
- Verify our settings still work
Relevant test scenarios
- [x] Changes should be tested with the browser console open
- [ ] Changes should be tested on different posts/pages/taxonomies/custom post types/custom taxonomies
- [x] Changes should be tested on different editors (Block/Classic/Elementor/other)
- [ ] Changes should be tested on different browsers
- [ ] Changes should be tested on multisite
Test instructions for QA when the code is in the RC
- [x] QA should use the same steps as above.
QA can test this PR by following these steps:
Impact check
This PR affects the following parts of the plugin, which may require extra testing:
- In an ideal world, only the UI library. But in the current monorepo, other packages too.
UI changes
- [ ] This PR changes the UI in the plugin. I have added the 'UI change' label to this PR.
Other environments
- [ ] This PR also affects Shopify. I have added a changelog entry starting with
[shopify-seo]
, added test instructions for Shopify and attached theShopify
label to this PR.
Documentation
- [ ] I have written documentation for this change.
Quality assurance
- [x] I have tested this code to the best of my abilities.
- [ ] During testing, I had activated all plugins that Yoast SEO provides integrations for.
- [ ] I have added unit tests to verify the code works as intended.
- [ ] If any part of the code is behind a feature flag, my test instructions also cover cases where the feature flag is switched off.
- [ ] I have written this PR in accordance with my team's definition of done.
- [ ] I have checked that the base branch is correctly set.
Innovation
- [x] No innovation project is applicable for this PR.
- [ ] This PR falls under an innovation project. I have attached the
innovation
label. - [ ] I have added my hours to the WBSO document.
Fixes https://github.com/Yoast/ui-library-storybook/issues/13
Pull Request Test Coverage Report for Build 618bb50f9724c513bfb723d6e206a2e57c368d9d
Details
- 0 of 0 changed or added relevant lines in 0 files are covered.
- No unchanged relevant lines lost coverage.
- Overall coverage remained the same at 53.781%
Totals | |
---|---|
Change from base Build 387fe1687881c27dbb22f31009605b4a0b8bbe98: | 0.0% |
Covered Lines: | 29529 |
Relevant Lines: | 55096 |
💛 - Coveralls
I ran yarn workspace @yoast/ui-library build
and compared the output of packages/ui-library/components/autocomplete-field/index.js
with the currently published version of the file. What used to be build were ES Modules with transpiled JSX. Now it's CommonJS with transpiled JSX. I expect that might affect consumers?
new | vs | published |
---|---|---|
Besides some merge conflicts, these are my changes during/after rebase:
Replace babel config needed adaptation for added sourceMaps :
- Using
api.env
- Removing previous
api.cache
due to that picking it up fromapi.env
- Could've contained fixup adjust to new babel config
- Could've contained fixup remove storycomponents
Upgrade @storybook/* and other commits that add Storybook packages
- Bumped the version from
7.6.10
to7.6.17
Continuing the AT from here.
Verify our classic editor integration still works: Done and all good ✅ Verify our Elementor integration still works: Done and all good ✅
This gave quite some trouble, with help from Dennis we've managed to find the root cause: __forceInitialArgs
always set to true
inside the docs page. Meaning Storybook does this on purpose, making the docs less interactive than we would want.
I've implemented a workaround for all our stories in this commit.
I did not fix the AutocompleteField selectedLabel story, it is more a display of that selectedLabel
overrides whatever value was selected. Though I agree that is currently not clear, I'm leaving this for our future selves (created a task).
I did adjust the SelectField and Select children prop stories to auto-adjust the selectedLabel
in the story. As that has the options
alternative.
Please note that only stories / Storybook behavior was adjusted in the last ~two~ four commits. So no need to re-test the plugin side!
Edit: ⚠️ force pushed to get rid of a merge (d-claassen/ui-library-form-patterns
) that was meant to try only locally 😅