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

chore: update mobx from 5.15.4 to 6.13.5 and mobx-react from 6.1.8 to 9.1.1 inside @packages/reporter #30514

Merged
merged 4 commits into from
Nov 4, 2024

Conversation

AtofStryker
Copy link
Contributor

@AtofStryker AtofStryker commented Nov 1, 2024

Additional details

Updates the Cypress Reporter to use updated mobx and mobx-react. This is a prerequisite to updating to React 17 (or at least done in isolation outside of updating to React 17 (see #30510))

To do this, I ran npx mobx-undecorate --keepDecorators to do the code migration and followed the mobx migration guide

mobx-4.mp4
mobx-5.mp4

Steps to test

All tests should serve as a regression test, but we likely want to do some performance benchmark testing with honeycomb and make sure no issues there are present. It is also a good idea to take the published binaries and run them as a smoke test on your machine to make sure everything looks good

How has the user experience changed?

PR Tasks

@AtofStryker AtofStryker self-assigned this Nov 1, 2024
@AtofStryker AtofStryker added the pkg/reporter This is due to an issue in the packages/reporter directory label Nov 1, 2024
Copy link

cypress bot commented Nov 1, 2024

cypress    Run #58116

Run Properties:  status check passed Passed #58116  •  git commit 5cca87a564: add changelog entry [run ci]
Project cypress
Branch Review chore/update_reporter_mobx
Run status status check passed Passed #58116
Run duration 17m 41s
Commit git commit 5cca87a564: add changelog entry [run ci]
Committer AtofStryker
View all properties for this run ↗︎

Test results
Tests that failed  Failures 0
Tests that were flaky  Flaky 0
Tests that did not run due to a developer annotating a test with .skip  Pending 26
Tests that did not run due to a failure in a mocha hook  Skipped 0
Tests that passed  Passing 770
View all changes introduced in this branch ↗︎
UI Coverage  66.27%
  Untested elements 26  
  Tested elements 55  
Accessibility  96.17%
  Failed rules  0 critical   4 serious   1 moderate   0 minor
  Failed elements 205  

@AtofStryker
Copy link
Contributor Author

AtofStryker commented Nov 1, 2024

TODO: need to check the test replay

@AtofStryker AtofStryker marked this pull request as ready for review November 4, 2024 15:15
chore: update mobx-react from v7 to v9 [run ci]

bump cache [run ci]
…in production mode, bundles the cjs production dependency of mobx (mobx.cjs.production.min.js) into cypress_runner.js (in the runner package), but @packages/app (bundled via vite), bundles mobx.esm.js, which is NOT minimized and is expecting the global store/state set by the app to also not be minimized. [run ci]
@AtofStryker AtofStryker force-pushed the chore/update_reporter_mobx branch from 72a5e4b to 5cca87a Compare November 4, 2024 15:23
@@ -5,6 +5,7 @@ _Released 11/5/2024 (PENDING)_

**Dependency Updates:**

- Updated `mobx` from `5.15.4` to `6.13.5` and `mobx-react` from `6.1.8` to `9.1.1`. Addresses [#30509](https://github.com/cypress-io/cypress/issues/30509).
Copy link
Member

Choose a reason for hiding this comment

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

@AtofStryker These don't need to be in the changelog, it's a chore.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I figured it was best to keep it just in case someone runs into an issue and isn't sure where it came from. I don't think it fails the verify changelog job but I can remove it if you'd like.

Copy link
Member

@jennifer-shehane jennifer-shehane left a comment

Choose a reason for hiding this comment

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

Do we want to add these suggestions from the migration?

The MobX default configuration has become more strict. We recommend to adopt the new defaults after completing the upgrade, check out the Configuration {🚀} section. During migration, we recommend to configure MobX in the same way as it would be in v4/v5 out of the box: import {configure} from "mobx"; configure({ enforceActions: "never" });. After finishing the entire migration process and validating that your project works as expected, consider enabling the flags computedRequiresReaction, reactionRequiresObservable and observableRequiresReaction and enforceActions: "observed" to write more idiomatic MobX code.

@AtofStryker
Copy link
Contributor Author

Do we want to add these suggestions from the migration?

The MobX default configuration has become more strict. We recommend to adopt the new defaults after completing the upgrade, check out the Configuration {🚀} section. During migration, we recommend to configure MobX in the same way as it would be in v4/v5 out of the box: import {configure} from "mobx"; configure({ enforceActions: "never" });. After finishing the entire migration process and validating that your project works as expected, consider enabling the flags computedRequiresReaction, reactionRequiresObservable and observableRequiresReaction and enforceActions: "observed" to write more idiomatic MobX code.

I think we might want to consider it once we are in a fully stable over to react 18. Right now we toggle the enforceActions in our driver tests to mutate certain types of behavior to test. But this doesn't fully work in later versions of React (since we are mutating internals) and I think that might be the time to do it?

@@ -15,13 +15,14 @@ export interface SessionProps extends InstrumentProps {
}

export default class Session extends Instrument {
@observable name: string
Copy link
Contributor

Choose a reason for hiding this comment

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

Why did this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

its declared in the Instrument class so its inherited. declaring it twice in mobx 6 throws an error

@AtofStryker AtofStryker requested a review from mschile November 4, 2024 21:23
@AtofStryker AtofStryker merged commit 3c6a092 into develop Nov 4, 2024
129 of 131 checks passed
@AtofStryker AtofStryker deleted the chore/update_reporter_mobx branch November 4, 2024 21:29
@cypress-bot
Copy link
Contributor

cypress-bot bot commented Nov 5, 2024

Released in 13.15.2.

This comment thread has been locked. If you are still experiencing this issue after upgrading to
Cypress v13.15.2, please open a new issue.

@cypress-bot cypress-bot bot locked as resolved and limited conversation to collaborators Nov 5, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
pkg/reporter This is due to an issue in the packages/reporter directory
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update @packages/reporter and @packages/app to use mobx v6 and mobx-react v9
4 participants