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

Upgrade typescript to 4.9.5 and jest to 29.7.0 (and related packages) #1884

Merged
merged 36 commits into from
Jul 30, 2024

Conversation

languy
Copy link
Member

@languy languy commented Jun 27, 2024

Preview this branch
This change is driven by the upgrade of @fluentui/react-components to 9.54.2 which wouldn't compile anymore with our current version of typescript (4.3.5). For reference, the error was a new syntax for defining a type within an export on one of fluent UI dependent package: an easy fix would have been to patch that file, but I felt that upgrading typescript was eventually inevitable. I picked the highest version of typescript 4, avoiding more work probably required for an upgrade to typescript 5 and also because fluent UI appears to be using 4.9.5.

Changes related to typescript upgrade from 4.3.5 to 4.9.5 (most of the upgrades were required to compile)

  • @phosphor/virtualdom (used by notebooks) does not compile anymore and isn't actively maintained: MediaStreamErrorEvent has been removed in typescript 4.4. Added a patch to use Event instead.
  • Upgrade react-i18next which doesn't support the wait config option anymore since v8.3.3. See warning in code and release notes.
  • Upgrade typedoc
  • Removed some Notebook-related files from strict compile (probably due to nteract packages not strict compiling anymore)

The typescript upgrade requires upgrading jest (I took the latest version)

  • Upgrade html-loader to latest
    • root option to specify where /image_filename_here path doesn't exist anymore in webpack.config.js. Adding a resolve.roots entry for images breaks the other paths, so I fixed the image paths in our .html files.
    • All other options are on by default, so the options fields goes away
  • Starting in Jest 27, testingEnvironment is node by default, so we must switch to jsdom (to get the window object, etc) which doesn't ship with jest by default. (I tried mocking the window object in globals jest config, but couldn't figure out how to mock window.Date). I installed jest-environment-dom which doesn't like the hack that we put in for canvas, so I replaced it with the proper canvas package.
  • Jest moved away from jasmine to its own test/assertion infra (jest-circus) in the latest version. For reference, there a way to configure jest to still use jasmine by setting test environment to jasmine2, but instead I migrated the handful of tests to jest test.
  • Replace old html-loader-jest with jest-html-loader
  • Issue with version 8 of uuid package (required by our sdk (among other dependencies)).
  • Upgraded babel packages used by jest
  • The jest snapshots must be updated because jest now save objects without the keyword Object anymore, same for Array for arrays.
  • import "@testing-library/jest-dom/extend-expect"; is now be called as import "@testing-library/jest-dom";
  • Disable e2e self-serve test for sql on webkit only: for some reason, playwright is not opening the dropdown (see comments in test/sql/selfServeExample.spec.ts).
  • Add Google Chrome and MS Edge to list of device browsers in playwright config.

Various related changes

  • Upgraded @fluentui/react (v8) to latest

Notes

@languy languy changed the title Upgrade typescript to 4.9.5 and jest to 29.7.0 Upgrade typescript to 4.9.5 and jest to 29.7.0 (and related packages) Jun 28, 2024
analogrelay
analogrelay previously approved these changes Jul 2, 2024
Copy link
Member

@analogrelay analogrelay left a comment

Choose a reason for hiding this comment

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

Looks good to me! I'm actually blocked on getting TypeScript updated for my existing PR, so getting this in would be great.

@analogrelay
Copy link
Member

I'm not sure why the "self serve" Playwright test failed, it only seems to have failed in webkit and seems like it might be a transient issue. I wonder if this test is worth keeping?

@languy languy marked this pull request as ready for review July 8, 2024 15:11
@languy languy requested a review from a team as a code owner July 8, 2024 15:11
@languy
Copy link
Member Author

languy commented Jul 8, 2024

I'm not sure why the "self serve" Playwright test failed, it only seems to have failed in webkit and seems like it might be a transient issue. I wonder if this test is worth keeping?

The issue seems to be persisting: the dropdown doesn't seem to open after a click according to the test snapshot. The error message displayed by playwright seems to suggest that there's a <div> (presumably one of the labels based on the class name) that stands in the way and prevents the event to go to the dropdown. I tried everything under the sun: add some delay, force a click, resize viewport, use selectOption but no luck so I disabled the test for webkit (since it works for other browsers).
Now everything is green and the PR is ready to go.

@analogrelay
Copy link
Member

so I disabled the test for webkit (since it works for other browsers). Now everything is green and the PR is ready to go.

This seems reasonable to me. Not something we should get used to doing, but this seems like a fairly clear edge-case and I'd rather disable the test than have it continue to cause problems.

Copy link
Member

@analogrelay analogrelay left a comment

Choose a reason for hiding this comment

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

Looks good to me. Would love to get this in as soon as we can (after RBAC stuff lands and is stable), to unblock some other work.

I also really appreciate the detailed justification for the skipped test 👍🏻 👍🏻

@analogrelay analogrelay merged commit bcd8b72 into master Jul 30, 2024
20 checks passed
@analogrelay analogrelay deleted the users/languy/upgrade-typescript-to-4.9.5 branch July 30, 2024 22:41
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.

4 participants