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

Update deprecated and outdated devDependencies #7590

Open
wants to merge 32 commits into
base: master
Choose a base branch
from

Conversation

wgoehrig
Copy link
Member

@wgoehrig wgoehrig commented Jan 22, 2025

It's been a while since we've tried to update many of our devDependencies, and as a result the number of deprecation and unmet peer warnings has grown and grown. Rather than attempting to update all devDependencies en masse, I've started with the deprecation warnings and worked backwards. With these changes, the 50+ lines of warnings on rush install are now down to:

 WARN  7 deprecated subdependencies found: [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected]
...
 WARN  Issues with peer dependencies found
../../core/frontend
└─┬ @vitest/browser 2.1.0
  └─┬ @vitest/mocker 2.1.0
    └── ✕ unmet peer vite@^5.0.0: found 6.0.6

../../full-stack-tests/core
└─┬ @itwin/electron-authorization 0.19.8
  └── ✕ unmet peer electron@">=23.0.0 <34.0.0": found 34.0.0

The following dependencies are preventing us from updating/removing those last 7 deprecated packages:

  • typemoq1 has not been maintained at all in nearly 7 years; we should migrate away from it ASAP.
  • json-schema-faker2 has newer versions available, but I'm concerned that updating may break or introduce flakiness in presentation tests.
  • typescript-json-schema3 is a dependency of @itwin/presentation-common and is now in "maintenance mode". We should consider switching to an alternative package or opening a PR to help them update.
  • nyc34 already has a PR open to update glob and rimraf
  • babel-plugin-istanbul3 already has a PR open to update test-exclude
  • azurite45 has a PR open for uuid, but nothing currently for rimraf or @azure/ms-rest-js (although there is an issue open for @azure/ms-rest-js)

Updating jsdom led me to remove jsdom-global since it breaks atob in jsdom v23 (and also hasn't been updated in 8 years). In a few cases I still had to stick some globals from jsdom into node's global scope but the jsdom docs are very clear that this is an anti-pattern. We should probably consider dropping jsdom entirely and moving those tests to vitest-browser.

Footnotes

  1. typemoc depends on both [email protected] and [email protected]

  2. json-schema-faker depends on [email protected]

  3. typescript-json-schema, nyc, and babel-plugin-istanbul (via test-exclude) all depend on [email protected] (which itself depends on [email protected]) 2 3

  4. azurite and nyc (both directly and via spawn-wrap) depend on [email protected] (which itself depends on [email protected] and, indirectly, [email protected]) 2

  5. azurite (both directly and via @azure/ms-rest-js) depends on [email protected]

@wgoehrig wgoehrig marked this pull request as ready for review February 5, 2025 23:47
@wgoehrig wgoehrig requested review from a team and bbastings as code owners February 5, 2025 23:47
Copy link
Member

@aruniverse aruniverse left a comment

Choose a reason for hiding this comment

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

thx for volunteering to do this Bill.
i hope we can drop nyc, instabul, typemoq as we move to vitest, and probably wouldnt want to spend any further time updating those deps

common/config/rush/pnpm-config.json Outdated Show resolved Hide resolved
common/config/rush/.pnpmfile.cjs Show resolved Hide resolved
extensions/test-extension/package.json Outdated Show resolved Hide resolved
Copy link
Member

@grigasp grigasp left a comment

Choose a reason for hiding this comment

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

Approving presentation changes.

For the 3 dependencies likely coming only from our packages:

typemoq[^1] has not been maintained at all in nearly 7 years; we should migrate away from it ASAP.

While I agree we should avoid using it in all new tests (and we do), and it would be great to get rid of it, is having it under devDependencies really that big of a problem? We have a huge set of tests which are using the library, and switching to another library would be a huge undertaking. In addition, the tests are for code that's gradually being deprecated and will go away in future releases. With all that in mind, I'd hate to waste anyone's time on this, unless it's a real issue.

json-schema-faker[^2] has newer versions available, but I'm concerned that updating may break or introduce flakiness in presentation tests.

I'm going to drop it, it's not necessary.

typescript-json-schema[^3] is a dependency of @itwin/presentation-common and is now in "maintenance mode". We should consider switching to an alternative package or opening a PR to help them update.

#7659

Copy link
Contributor

mergify bot commented Feb 6, 2025

This pull request is now in conflicts. Could you fix it @wgoehrig? 🙏
To fixup this pull request, you can check out it locally. See documentation: https://help.github.com/articles/checking-out-pull-requests-locally/

@wgoehrig
Copy link
Member Author

wgoehrig commented Feb 7, 2025

@tcobbs-bentley looks like the build is failing due to missing support for a Unicode character class in our iOS environment. Is ICU somehow disabled on mobile or something? If not, any other ideas?

FWIW, we can revert the sinon update that caused this, but I'm concerned that it's a matter of time before another dependency runs into something like this - seems like we should expect regexes like this to be more common as more and more ReDoS vulnerabilities are found and fixed...

@tcobbs-bentley
Copy link
Member

@wgoehrig ICU is missing from the mobile builds of Node (both iOS and Android). Unfortunately, the way that we build Node makes it difficult to include. The V8 build is done completely separately from the rest of the build which include ICU4C, and it needs ICU support in order for Node to support ICU. Fixing this may be possible, but I'm not sure how to do so in a reasonable way.

We have run into this problem before and made sure that the problematic code didn't get accessed in mobile builds. The problem with this specific instance is that Node throws an exception while parsing the JS due to the hard-coded RegEx using functionality that only works when ICU is loaded. So that code has to be completely excluded from the mobile build.

I think that a discussion is probably needed between multiple people who have a firm understanding of Bentley buildology if we want to get ICU support into our mobile Node builds.

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

Successfully merging this pull request may close these issues.

8 participants