Upgrade Ariakit #64066
Upgrade Ariakit #64066
Conversation
|
Size Change: +6.79 kB (+0.39%) Total Size: 1.77 MB
ℹ️ View Unchanged
|
|
This was kind of a PITA to pull off, since it required reverting the lockfile, doing a I was extremely careful in double checking that the fixes take the expected effect and everything looks correct. This allowed me to figure out when the mistery new test failures come, and that's at version v0.4.6. Will be working on those next, until we're at latest (v0.4.7). Please @WordPress/gutenberg-components read through the new, much shorter description since we need to discuss some last details and I would also appreciate some help with testing and generally making sure I didn't miss anything important. |
|
Thank you, Dani! As mentioned in #62947, we should probably look at cleaning up the custom
That could be a way to improve the devX in the components package (I'm always in favor of consistency). But we should also consider consumers of the components package (both other Gutenberg packages, and third-party consumers), who may end up struggling with the same failures. How can we help them? Considering that
For more context, the main change here is that "users are responsible for making anchors accessible" (source). Judging by previous conversations (example), we should be ok with this change. Although I would still have a pass and make sure that the removal of the tooltip description won't cause anchors to suddenly become inaccessible (and we should fix that by adding the information back in other ways, like with visually hidden text in the anchor). |
Yep, ideally by contributing to Ariakit which is currently being discussed (ariakit/ariakit#3939 (comment)). It's on the TODO list!
+1
I would suggest being consistent: whatever we do in the components package is what we recommend on the outside. For now, that'd be switching to Note that, while it is indeed under the
This could be a good strategy in that eventuality, yes! However, that seems like a future issue that's not even completely guaranteed to exist, so I think we're fine for now with:
What do you think?
Great to know! Agree, we should do a pass. |
This reverts commit 7a43523.
|
I added the 
This is the output: https://pastebin.com/raw/RH2WGREJ (144 errors in the components package) I reverted this change (412f612) to keep this PR focused on the upgrade itself, this should be done in a follow-up, but wanted to see how this would look. cc @ciampo |
DaniGuardiola
added
[Type] Code Quality
packages/components/src/utils/tmp-ariakit-test-render-replacement.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Doing some testing in the editor and in Storybook today (and will do some tomorrow).
Initial observation: changing the color in a color picker seems to have become clunkier:
Trunk:
Screen.Recording.2024-07-31.at.18.57.21.mov
This branch:
Screen.Recording.2024-07-31.at.18.56.13.mov
|
@tyxla just need to go through the remaining todos. If nothing else comes up, changelog will be the only change. |
|
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. |
|
Checked components for any potential remaining animation issues but couldn't find any. Also did some general testing in the Storybook and Gutenberg and nothing looks broken. Undrafting, get your final reviews in! :) @WordPress/gutenberg-components |
|
I'm unable to reproduce this anymore 🎉 (not sure what was going on) |
There was a problem hiding this comment.
Thanks for the hard work on it @DaniGuardiola 🙌
I gave this a thorough amount of testing in Storybook, the site editor, and the post editor and didn't spot any regressions. ✅
The code looks good! 🚀 Thanks for pointing out all the changes and follow-ups in such detail!
The navigate metric seems to have doubled, but it's possible that it's a false alarm:
Let's keep an eye on performance metrics after merging this one, just in case. ⏲️
Also, let's monitor unit test builds for flakiness ❄️ in the next couple of days.
|
Filed #64205 to clean up the arbitrary |
Continuation of #62947 (dependencies got messed up)
To do
Besides upgrading to the latest version and fixing all remaining failed tests, there are some other items to complete.
aria-describedBychanges: add temporary compatibility fix and revert test changes.renderchanges to upstream@ariakit/test(returningrerenderfrom the output of the wrappedReactTestingLibrary.render). See Outputrerenderfrom@ariakit/test's render. ariakit/ariakit#3981@ariakit/test'srenderfunction.@ariakit/test'srenderand user events in all components package tests? (e.g.noRestrictedImports)History by version (unit tests)
v0.3.x (from v0.3.13)
At v0.3.13, tests fail:
The reason is tests need to give some components a bit more time to render since Ariakit uses some timers under the hood. Fixed (e52022c) by replacing RTL's
renderwith@ariakit/test'srender- including the local temporary fork withrerendersupport (ffe7928).After addressing, all tests pass in all v0.3.x versions.
0.4.x
This diff from the previous PR has information about all root causes for test failures.
Essentially, we're impacted by three Ariakit changes in v0.4.0:
After addressing, all tests pass between v0.4.0 and v0.4.5.
0.4.6 (287c22f, failing tests, fixed in 27218de)
In this version, there are 3-5 new test failures (2 seem to be flaky):
Similarly to the
renderwait issues, this is due to not properly waiting for some internal timers to complete. This can be remediated by replacing RTL's event triggers with ones from@ariakit/test.After addressing (27218de), all tests pass in this version.
0.4.7 - latest (94deccd)
All tests pass and we're on latest now! 🎉
Follow-ups
These are tasks to be done after this PR lands. I'm adding stuff that seems to have some consensus here, and unless I hear different I plan to go through with them. Please speak up if you have any arguments against these decisions! For more context on some of these, look for "resolution" notes in the "To do" section.
@ariakit/test'srenderand event trigger utilities in all components package tests (see Upgrade Ariakit #64066 (comment) and Upgrade Ariakit #64066 (comment)) - Components: consistently userenderfrom @ariakit/test #64180aria-describedBychanges:renderin appropriate place.@wordpress/components.sleepcalls - Components: Cleanup flaky unit testsleep()hacks #64205@ariakit/testfrom RTL (context).