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

updated react and react-dom #899

Closed
wants to merge 6 commits into from

Conversation

0x00b1
Copy link

@0x00b1 0x00b1 commented Sep 7, 2022

Hi, @JackUrb! I needed to update react and react-dom for the compression task. Let me know if this works.

cc: @mmuckley

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Sep 7, 2022
@JackUrb
Copy link
Contributor

JackUrb commented Sep 9, 2022

@pringshia any thoughts on how we deal with react versions in Mephisto? Has been an issue for other projects as well

@pringshia
Copy link
Contributor

pringshia commented Sep 9, 2022

This PR includes quite a bit of package version updates which I'm hesitant on merging right away, as there could be unintended behavior, the currently breaking tests aside. To back up a bit, what part of the prior version of react / react-dom was causing issues? Also I would advise to break down the PR to introduce the updates across the various packages incrementally instead of all-at-once.

@0x00b1
Copy link
Author

0x00b1 commented Sep 11, 2022

To back up a bit, what part of the prior version of react / react-dom was causing issues? Also I would advise to break down the PR to introduce the updates across the various packages incrementally instead of all-at-once.

Lack of useDeferredValue and useTransition. Also the ability to use @mui/material 5.10.4.

@@ -24,9 +24,9 @@ jobs:
steps:
# Checks-out your repository under $GITHUB_WORKSPACE, so your job can access it
- uses: actions/checkout@v2
- uses: actions/setup-node@v2
- uses: actions/setup-node@v3
Copy link
Contributor

Choose a reason for hiding this comment

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

This is causing tests to fail due to the following issue: actions/setup-node#317

@pringshia
Copy link
Contributor

pringshia commented Sep 12, 2022

Given the node version bump in the GH runner, seems like we'll also need to update yarn to version 3.2.2+ to fix the issue seen here:

➤ YN0000: │ @parcel/watcher@npm:2.0.4 STDERR node:internal/errors:477
  ➤ YN0000: │ @parcel/watcher@npm:2.0.4 STDERR     ErrorCaptureStackTrace(err);
  ➤ YN0000: │ @parcel/watcher@npm:2.0.4 STDERR     ^
  ➤ YN0000: │ @parcel/watcher@npm:2.0.4 STDERR 
  ➤ YN0000: │ @parcel/watcher@npm:2.0.4 STDERR Error [ERR_LOADER_CHAIN_INCOMPLETE]: "file:///home/runner/work/Mephisto/Mephisto/.pnp.loader.mjs 'resolve'" did not call the next hook in its chain and did not explicitly signal a short circuit. If this is intentional, include `shortCircuit: true` in the hook's return.
  ➤ YN0000: │ @parcel/watcher@npm:2.0.4 STDERR     at new NodeError (node:internal/errors:387:5)
  ➤ YN0000: │ @parcel/watcher@npm:2.0.4 STDERR     at ESMLoader.resolve (node:internal/modules/esm/loader:[85](https://github.com/facebookresearch/Mephisto/runs/8275528832?check_suite_focus=true#step:9:89)2:13)
  ➤ YN0000: │ @parcel/watcher@npm:2.0.4 STDERR     at async ESMLoader.getModuleJob (node:internal/modules/esm/loader:431:7)
  ➤ YN0000: │ @parcel/watcher@npm:2.0.4 STDERR     at async Promise.all (index 0)
  ➤ YN0000: │ @parcel/watcher@npm:2.0.4 STDERR     at async ESMLoader.import (node:internal/modules/esm/loader:533:24)
  ➤ YN0000: │ @parcel/watcher@npm:2.0.4 STDERR     at async loadESM (node:internal/process/esm_loader:91:5)
  ➤ YN0000: │ @parcel/watcher@npm:2.0.4 STDERR     at async handleMainPromise (node:internal/modules/run_main:65:12) {
  ➤ YN0000: │ @parcel/watcher@npm:2.0.4 STDERR   code: 'ERR_LOADER_CHAIN_INCOMPLETE'
  ➤ YN0000: │ @parcel/watcher@npm:2.0.4 STDERR }
  ➤ YN0009: │ @parcel/watcher@npm:2.0.4 couldn't be built successfully (exit code 1, logs can be found here: /tmp/xfs-d4a4833f/build.log)

@pringshia pringshia added this to the Mephisto X.0 milestone Sep 12, 2022
@pringshia pringshia mentioned this pull request Oct 3, 2022
@pringshia
Copy link
Contributor

pringshia commented Oct 3, 2022

Following up on this, I think you should just be able to relax the peerDependency requirement for mephisto-task instead of doing such a broad version bump across the codebase. For example: https://github.com/palantir/blueprint/blob/b0d500511217db6590e86ccb9627385b4fd153a5/packages/core/package.json#L63-L64

@pringshia
Copy link
Contributor

Resolved with #905

@pringshia pringshia closed this Jan 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants