wordpress-seo icon indicating copy to clipboard operation
wordpress-seo copied to clipboard

Upgrade UI library dependencies

Open igorschoester opened this issue 1 year ago • 2 comments

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 with React 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 the cols 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 the yarn.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: image
      • After: image

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 the Shopify 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

igorschoester avatar Dec 13 '23 14:12 igorschoester

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 Coverage Status
Change from base Build 387fe1687881c27dbb22f31009605b4a0b8bbe98: 0.0%
Covered Lines: 29529
Relevant Lines: 55096

💛 - Coveralls

coveralls avatar Jan 25 '24 07:01 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
afbeelding afbeelding

d-claassen avatar Jan 26 '24 08:01 d-claassen

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 from api.env

Adjust to new babel config

Remove StoryComponents

Upgrade @storybook/* and other commits that add Storybook packages

  • Bumped the version from 7.6.10 to 7.6.17

igorschoester avatar Mar 07 '24 12:03 igorschoester

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 ✅

leonidasmi avatar Mar 19 '24 08:03 leonidasmi

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 😅

igorschoester avatar Mar 23 '24 12:03 igorschoester