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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Jun 15, 2023
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
599b11f
Upgrade Node.js to v18 on CircleCI, CodeSandbox and Netlify
ZeeshanTamboli Jun 13, 2023
006a91e
use new window.Event
ZeeshanTamboli Jun 14, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .circleci/config.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ defaults: &defaults
REACT_DIST_TAG: << parameters.react-dist-tag >>
working_directory: /tmp/mui
docker:
- image: cimg/node:14.21
- image: cimg/node:18.16
# CircleCI has disabled the cache across forks for security reasons.
# Following their official statement, it was a quick solution, they
# are working on providing this feature back with appropriate security measures.
Expand Down
2 changes: 1 addition & 1 deletion .codesandbox/ci.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
{
"buildCommand": "build:codesandbox",
"installCommand": "install:codesandbox",
"node": "14",
"node": "18",
"packages": [
"packages/x-license-pro",
"packages/grid/x-data-grid",
Expand Down
2 changes: 1 addition & 1 deletion netlify.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
command = "yarn docs:build && yarn docs:export"

[build.environment]
NODE_VERSION = "14"
NODE_VERSION = "18"
NODE_OPTIONS = "--max_old_space_size=4096"
# Not using `playwright` when building docs.
PLAYWRIGHT_SKIP_BROWSER_DOWNLOAD = "1"
Expand Down
2 changes: 1 addition & 1 deletion packages/x-date-pickers/src/DateField/tests/editing.DateField.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 •
edited
Loading

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 •
edited
Loading

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. 🤔

bubbles: true,
cancelable: true,
composed: true,
Expand Down