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

[core] Upgrade Node.js to v18 on CircleCI, CodeSandbox and Netlify #9319

Conversation

ZeeshanTamboli
Copy link
Member

Similar to mui/material-ui#37173.

@ZeeshanTamboli ZeeshanTamboli added core Infrastructure work going on behind the scenes dependencies Update of dependencies labels Jun 13, 2023
@mui-bot
Copy link

mui-bot commented Jun 13, 2023

Netlify deploy preview

Netlify deploy preview: https://deploy-preview-9319--material-ui-x.netlify.app/

Updated pages

No updates.

These are the results for the performance tests:

Test case Unit Min Max Median Mean σ
Filter 100k rows ms 615.7 1,109 615.7 838.7 198.127
Sort 100k rows ms 571.9 1,075.5 571.9 895.74 176.334
Select 100k rows ms 223.5 312.4 280.6 280.3 32.018
Deselect 100k rows ms 142.8 293.3 208.7 208.36 53.668

Generated by 🚫 dangerJS against 006a91e

@ZeeshanTamboli ZeeshanTamboli marked this pull request as ready for review June 14, 2023 07:41
@ZeeshanTamboli ZeeshanTamboli changed the title [WIP] Upgrade Node.js to v18 on CircleCI, CodeSandbox and Netlify Upgrade Node.js to v18 on CircleCI, CodeSandbox and Netlify Jun 14, 2023
@ZeeshanTamboli ZeeshanTamboli changed the title Upgrade Node.js to v18 on CircleCI, CodeSandbox and Netlify [core] Upgrade Node.js to v18 on CircleCI, CodeSandbox and Netlify Jun 14, 2023
@@ -647,7 +647,7 @@ describe('<DateField /> - Editing', () => {
describeAdapters('Pasting', DateField, ({ adapter, render, renderWithProps, clickOnInput }) => {
const firePasteEvent = (input: HTMLInputElement, pastedValue: string) => {
act(() => {
const clipboardEvent = new Event('paste', {
const clipboardEvent = new window.Event('paste', {
Copy link
Member Author

Choose a reason for hiding this comment

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

I had to change this because the tests were failing with the following error:

TypeError: Failed to execute 'dispatchEvent' on 'EventTarget': parameter 1 is not of type 'Event'.

I think that the availability of the Event constructor without the window prefix can vary depending on the Node.js version and the specific environment setup. Having this ensures that it is created within the jsdom env, making it compatible with the simulated DOM.

Somewhat related to the discussion in the GitHub issue: jsdom/jsdom#3331.

I also attempted to upgrade to JSDom version 22.1.0 (#8913), but unfortunately, it did not resolve the problem.

Copy link
Member

Choose a reason for hiding this comment

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

It's strange that numerous tests on Data Grid side using new Event('scroll') pass, but changing this test to new Event('paste') (without the second argument) still results in failures... 🤷

Copy link
Member Author

@ZeeshanTamboli ZeeshanTamboli Jun 14, 2023

Choose a reason for hiding this comment

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

@LukasTy Those tests are not running on JSDom. They are running only on browser.

      if (isJSDOM) {
        // Need layouting
        this.skip();
      }

Copy link
Member

Choose a reason for hiding this comment

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

🤦 I could swear that I did check for this and found at least one without the tests being skipped, before writing the comment...
But anyways, thank you for double-checking. 👌 🙇

Copy link
Member Author

@ZeeshanTamboli ZeeshanTamboli Jun 14, 2023

Choose a reason for hiding this comment

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

Maybe you missed checking the before hook for that test block 😅

    before(function beforeHook() {
      if (isJSDOM) {
        // Need layouting
        this.skip();
      }
    });

Copy link
Member

Choose a reason for hiding this comment

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

Could be. 🤔

@ZeeshanTamboli ZeeshanTamboli requested a review from a team June 14, 2023 07:53
Copy link
Member

@LukasTy LukasTy left a comment

Choose a reason for hiding this comment

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

Thank you for looking at this matter and resolving the test failure! 💯 👍
@flaviendelangle do you see any potential problems with the test change? 🤔

@@ -647,7 +647,7 @@ describe('<DateField /> - Editing', () => {
describeAdapters('Pasting', DateField, ({ adapter, render, renderWithProps, clickOnInput }) => {
const firePasteEvent = (input: HTMLInputElement, pastedValue: string) => {
act(() => {
const clipboardEvent = new Event('paste', {
const clipboardEvent = new window.Event('paste', {
Copy link
Member

Choose a reason for hiding this comment

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

It's strange that numerous tests on Data Grid side using new Event('scroll') pass, but changing this test to new Event('paste') (without the second argument) still results in failures... 🤷

Copy link
Member

@cherniavskii cherniavskii left a comment

Choose a reason for hiding this comment

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

Thank you!

@ZeeshanTamboli ZeeshanTamboli merged commit ecd64a4 into mui:master Jun 15, 2023
@ZeeshanTamboli ZeeshanTamboli deleted the upgrade-node-to-v18-ci-condesandbox-netlify branch June 15, 2023 05:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Infrastructure work going on behind the scenes dependencies Update of dependencies
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants