Skip to content
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

chore(#621): update non-breaking dependencies #623

Merged
merged 21 commits into from
Jul 22, 2024
Merged

Conversation

m5r
Copy link
Member

@m5r m5r commented Jul 16, 2024

Description

Update all dependencies at once is an important effort so I'm splitting this up in more manageable chunks, starting with non-breaking (or minimally breaking) dependencies

#621

Decisions made along the way:

  • keeping semantic-release on v22 instead of updating to latest v24 because starting with v23 they stopped supporting Node.js 18
  • not updating chai because chai-exclude still relies on chai <= 4 in their peerDependencies - note that this is changing, they recently merged a PR that resolves this, so we can expect a release soon allowing us to update chai then
  • keeping open on v8.4.2 instead of updating to latest v10 because starting with v9 it's an ESM-only package and ESLint is so outdated it can't parse the await import('package') syntax 🤦‍♂️
  • not updating xpath from v0.0.33 to v0.0.34 because no changelog is provided, I'm not sure what changed in that version and this is an important dependency for parsing XPath paths in forms
  • keeping @parcel/watcher on v2.1.0 because v2.2.0-2.4.0 have incompatibilities with npm 10.4 (see gyp: binding.gyp not found (...) while trying to load binding.gyp parcel-bundler/watcher#156) and v2.4.1 has a weird bug where the watcher hangs ~5% of the time when unsubscribing (see chore(#621): update non-breaking dependencies #623 (comment))

Code review items

  • Readable: Concise, well named, follows the style guide, documented if necessary.
  • Documented: Configuration and user documentation on cht-docs
  • Tested: Unit and/or integration tests where appropriate
  • Backwards compatible: Works with existing data and configuration. Any breaking changes documented in the release notes.

License

The software is provided under AGPL-3.0. Contributions to this project are accepted under the same license.

@m5r m5r requested a review from jkuester July 16, 2024 22:41
@m5r
Copy link
Member Author

m5r commented Jul 16, 2024

@jkuester the PR is pretty much ready for review. The last test I fixed shows diff when local is different from remote and the user requests a diff has a weird behavior where the terminal "color code prefixes" are part of the diff locally but not in the CI ever since I updated json-diff. I'll fix this first thing tomorrow but I don't expect it to be an important change

@m5r m5r marked this pull request as ready for review July 16, 2024 22:57
@jkuester
Copy link
Contributor

FYI, just opened a docs PR (medic/cht-docs#1460) for disabling the Sonar TODO rule. Seems to do more harm than good...

Copy link
Contributor

@jkuester jkuester left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🚀

@jkuester
Copy link
Contributor

@m5r it looks like this TODO rule fall into the annoying category of Sonar rules that do not seem to be ignorable via the normal inline means. 🤔

I guess we can either just remove the comments (since they are getting fixed anyway in #624) or you should be able to bypass them in the Sonar UI by "Accepting" them (hopefully if you are logged into Sonar you can see the drop-down to change the status from Open to Accepted?):

Screenshot from 2024-07-18 11-47-52

@m5r
Copy link
Member Author

m5r commented Jul 18, 2024

@jkuester Sure, I'll remove them in this branch 👍

@m5r
Copy link
Member Author

m5r commented Jul 18, 2024

Holding off to merge this branch. I noticed a regression in unit tests that I pinpointed to the unsubscribe of @parcel/watcher hanging in src/fn/watch-project.js.
I re-ran it locally with the version we were previously running with and it never hangs. I need to look into which version introduced this change, see if there is something we can do on our side to prevent it from hanging or possibly keep using an old version of this package. What's surprising is this wasn't a major version update, we were behind by a few minor versions only...

Here is an example run where it hanged for ~20 minutes until I canceled it. The weird thing is that canceling it didn't work immediately and it took a few minutes for GitHub to actually stop the run. Same thing happens in local where CTRL+C won't stop the process, the ^C signal doesn't even show up in the terminal, you have to kill -9 the node process to get it to stop.

m5r added 2 commits July 22, 2024 12:35
… have incompatibilities with npm 10.4 (see parcel-bundler/watcher#156) or the watcher hangs ~5% of the time when unsubscribing (see #623 (comment))
@m5r m5r merged commit 22b0d53 into main Jul 22, 2024
7 checks passed
@m5r m5r deleted the 621-update-non-breaking-deps branch July 22, 2024 10:51
@medic-ci
Copy link
Collaborator

🎉 This PR is included in version 4.1.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants