Upgrades Hydrogen to 2.0.0-beta #3051
Conversation
JoshBeveridge
changed the title
…ers the way certain Hydrogen attributes were used to avoid dynamic attribute variables, fixed a few syntax bugs in the original code.
patcon
added
the
dependencies
|
So far, I'm seeing the following speed improvements:
And the following size improvements:
I've noticed instances where running the docker script gets a sporadic error from Hydrogen's processing script though... it's inconsistent and seems to have to do with timing? |
|
Nice! But rough on the sporadic error. Since docker is our primary dev environment, what's "inconsistent" mean here? one in 5 runs? in 10? in 25? Do you have a proposal for paths forward for this PR in relation to those sporadic failures? If it helps you confirm a fix, we could create a temporary GitHub Action workflow for just your branch, that runs only hydrogen on loop for a number of times chosen so that it would likely trigger the bug if it exists? Or maybe it makes me sense for that workflow to be in your hydrogen repo, like a big ugly test :) (GitHub Actions can checkout another repo in a workflow) CI builds randomly showing as red feel like they're hurt the value of the end-to-end test suite we're trying to build 😢 |
|
@patcon So far it's been 1/3 runs. Docker takes forever to run, so I'm not convinced running it manually a couple times is an efficient use of time. I have little to no experience writing tests for rapid testing, so feel free to suggest/submit something if you already have an idea in mind. Edit: to be specific, the error is that PostCSS is being passed |
|
It's possible it's happening on your local docker, but won't happen on CI. For example, if it's related to the type of disk mount, like if CI uses something different that's maybe more consistent with writes 🤷 ) Might be worth resolving the merge conflicts so we can see how it builds in CI. (I'm seeing a 26s h2 build time in other docker runs on CI, so it seems maybe faster that your 42s local build. Not sure if that bodes well or poorly for the likelihood of this bug rearing it's head on CI..!) |
You could leave a script like this at #!/usr/bin/env bash
set -e
for i in {1..5}
do
echo "Hydrogen run #$i"
npm run h2-build --workspace common
doneAnd run it like this: If it doesn't trigger the bug, you might want to delete any outputs at the end of each loop to simulate a true fresh run. And if the bug doesn't show, my next step of debugging would be to also run the commands that run before h2 in the larger docker build scripts. So assuming it's setup.sh that's causing the error, ensure the commands before |
|
Welp, that merge didn't give me the results I was expecting 😅 Edit: missed converting some additional Hydrogen values that were added during the merge. Edit: so I think, after looking at the logs, that typos are causing the processing errors. Some of these I can and should check for in H2 proper, others might (or might not?) require linting (like double brackets on a query, e.g. Edit: while typos were a problem, fixing them didn't stop the arbitrary PostCSS error; I think the PostCSS scripting is trying to run before Docker has written the raw CSS file, so I'm going to look into artificially delaying this process on my system to see if I can recreate the conditions. I haven't experienced this yet because the write is immediate in all my testing environments Edit: Okay, so the arbitrary error was a direct result of Docker being super slow and PostCSS being run inside an asynchronous |
…he gitignore from a past PR, and removes Hydrogen dependencies from all workspaces except common.
…ell as to reduce PostCSS processing times.
This reverts commit a89890a.
…to admin or applicant UI after logging in
…gins to the close pool dialog, adds padding and consistent containers to the not authorized and can't find UIs
…ark backgrounds like table footers
|
@patcon summary: (All links are to Chromatic storybook build of 6592473)
|
|
Admin Candidate Details text overflow on smaller screen with menu open: |
…o a single column sooner than portrait tablet devices.
|
I think all of mine and Peter's feedback was addressed |
… dependabot changes and integrity hashes
Requirements
#3050
Hydrogen needs to be updated to
2.0.0so that we can significantly reduce the app build time (~40s down to <500ms), as well as to reduce both tech and design debt resulting from breaking changes.For now, this branch will upgrade to the latest stable beta, which we can safely merge into main. The actual
2.0.0release is only waiting on documentation to be updated.As a result, working with the beta will mean that Hydrogen's documentation will be out of date, but the new snippets file will provide a great starting place for differences in syntax.
Outstanding development tasks
Add translation code for the new skill accordion label (i.e.Hydrogen Upgrade Related Copy Changes #3694This skill has the following related experiences:)Add translation code for new null text on experience accordions (i.e.Hydrogen Upgrade Related Copy Changes #3694No skills have been linked to this experience yet.)Update the label for experience categories in the "filter by type" list so that category names are explicit (e.g. Personal => Personal experiences)Hydrogen Upgrade Related Copy Changes #3694Upgrade checklist
Heads up
There are going to be some muscle memory growing pains associated with this upgrade, particularly related to colors. The original color implementation was done piecemeal as we learned what colors the product was going to need, but this has been migrated and consolidated into a more complete approach using the new color configuration syntax.
Please refer to the upgrade guide provided in the
documentationdirectory. Check the guide out here.Review checklist
Standards and code related items
Common or sitewide UI
Admin UI
Talent search UI
Indigenous apprenticeship UI