Upgrade mobx, mobx-react and mobx-state-tree #2009

eatyourgreens · 2021-01-28T17:37:44Z

Update the NextJS apps to use [email protected], [email protected] and [email protected].
Upgrade the classifier to use [email protected], [email protected] and [email protected].
Upgrading mobx-react to 6.3 breaks the classifier tests, so I've left it alone for now.
Rewrite the project preferences store tests to use mobx.when. Previously, the tests were run without waiting for the project preferences to load, which broke in mobx-state-tree 3.16 and higher.
Also fixes a couple of project preferences tests which were accidentally nested inside another test (misplaced }) somewhere) and a typo in the same tests (projects.createActive(project.id) instead of projects.setActive(project.id).)

See #1839 for the major bug that appeared the last time I tried to upgrade the NextJS apps to mobx-react v6. I haven't been able to reproduce it on this branch, testing locally with a production build and PH-TESS.

Package:
app-content-pages
app-project
lib-classifier

Review Checklist

General

Are the tests passing locally and on Travis?
Is the documentation up to date?

Components

Has a storybook story been created or updated?
Is the component accessible?
- Can it be used with a screen reader? BBC guide to testing with VoiceOver
- Can it be used from the keyboard? WebAIM guide to keyboard testing
- Is it passing accessibility checks in the storybook?

Apps

Does it work in all major browsers: Firefox, Chrome, Edge, Safari?
Does it work on mobile?
Can you yarn panic && yarn bootstrap or docker-compose up --build and app works as expected?

Publishing

Is the changelog updated?
Are the dependencies updated for apps and libraries that are using the newly published library?

Post-merging

Did the app deploy to https://frontend.preview.zooniverse.org/projects/:project-name/:owner or https://frontend.preview.zooniverse.org/about?
Is the new feature working or bug now fixed?
- Is there a Talk or blog post written to announce the new feature(s)?
Is the design working across browsers (Firefox, Chrome, Edge, Safari) and mobile?
- Is this approved by our designer?
Is this ready for production deployment?

eatyourgreens · 2021-01-28T17:40:20Z

The last attempt to upgrade mobx-react broke classifications for live projects (#1839.) I'm hoping this will avoid that by installing the same version of mobx across the NextJS apps and the classifier.

eatyourgreens · 2021-01-29T09:16:21Z

I'm not seeing a recurrence of #1839 when I submit empty classifications on this branch.
http://local.zooniverse.org:3000/projects/nora-dot-eisner/planet-hunters-tess/classify/workflow/11235?demo=true

eatyourgreens · 2021-01-29T14:51:14Z

Should we upgrade mobx from 5 to 6? Migration guide.

eatyourgreens · 2021-01-29T16:34:12Z

UPP store model tests in the classifier are all broken, with mobx-state-tree > 3.15, because they don't wait for the UPP to load before running the expectations. 😞

EDIT: Classification works locally with MST 3.17. so I think the tests are the problem, not the store. The expectations run immediately after the project loads, but should wait for the user and UPP to load. This might be fixed by using mobx.when to run the tests.

Update the NextJS apps and the classifier to each use [email protected] and [email protected]

Bump mobx-react to 6.3 and mobx-state-tree to 3.17 for the NextJS apps. Upgrading the classifier breaks the tests for the store models, so I've held off on that.

eatyourgreens · 2021-02-01T12:19:14Z

packages/lib-classifier/src/store/UserProjectPreferencesStore.spec.js

+        return rootStore.userProjectPreferences.loadingState === asyncStates.success
+      }
+      when(
+        preferencesAreLoaded,


This delays running the tests until preferencesAreLoaded() is true.

Move duplicated code into a function. Run tests after the UPP has either loaded or errored, then check that the expected state is set.

shaunanoordin

PR Review

Projects: app-project lib-classifier app-content-pages

This PR is a major dependency update for our app's stores.

In general, testing looks good. I'm unable to break any of the main functionality of the app(s)
I'm unable to replicate the error in mobx-react v.6.1.4 breaks classifications without annotations on the first subject #1839 - submitting a Classification with no annotations works fine. This is good 👍

Testing

Part 1: Tested app-project on macOS10+Chrome88 on localhost:3000

Looking good:

With a workflow with 1 Single Question Task, I'm able to...
- ...answer one question and submit.
- ...provide no answer and submit
With a workflow with 2 Single Question Tasks, I'm able to...
- ...answer two questions and submit.
- ...answer one of the questions and submit.
- ...provide no answer and submit
- ...go back to a previous question to amend an answer
With a workflow with 1 Drawing Task (Point tool, Point tool), I'm able to...
- ...add, edit, and delete Point marks.
- ...provide no marks and submit.
- ...provide 1-5 marks and submit.
With a workflow with 1 Drawing Task (Point tool, Point tool), and 1 Text Task, I'm able to...
- ...add, edit, and delete Point marks.
- ...proceed to the Text Task, then go back to the Drawing Task to amend my earlier annotations.
  - ⚠️ [minor quirk] there's an oddity where going back to a previous drawing task causes the previous annotations to STILL appear as untouchable "previous marks", but this is extant even in master and not related to this PR. Probably a different bug to fix.
- ...provide any combination of answer/non-answers and submit

Additional testing on Safari12 and Win+Edge88 (but Edge is basically Chrome in a fancy hat at this moment so no surprise that there are no surprises here.)

Part 2: Tested lib-classifier on local.zooniverse.org:8080

Basic tests for lib-classifier looks fine.
Based on passing tests in Part 1, I did not feel the need to test the lib-classifier alone too thoroughly.

Part 3: Tested app-content-pages on macOS10+Chrome88 on localhost:3000

Content pages loaded fine and functioned normally.

Testing Limitations

I'm unable to test authentication/login on app-project on Chrome due to issues accessing https://local.zooniverse.org:3000 on my end. (Chrome spews SSL errors for my pseudo-localhost)
- On Safari (which is far more permissive with SSL infractions), however, I can confirm that auth/login works fine, for what that's worth.

Status

This PR looks good to me. 👍

shaunanoordin · 2021-02-01T20:36:47Z

⚠️ [minor quirk] there's an oddity where going back to a previous drawing task causes the previous annotations to STILL appear as untouchable "previous marks", but this is extant even in master and not related to this PR. Probably a different bug to fix.

I just realised you opened #2011 precisely for this. Reviewing that now!

eatyourgreens mentioned this pull request Jan 28, 2021

Project: remove classes for _app and page containers #1971

Merged

18 tasks

eatyourgreens force-pushed the upgrade-mobx-react branch from cc6574a to 60eade2 Compare January 29, 2021 09:17

eatyourgreens requested review from srallen and a team January 29, 2021 09:26

eatyourgreens added dependencies Pull requests that update a dependency file enhancement New feature or request labels Jan 29, 2021

eatyourgreens changed the title ~~Upgrade mobx and mobx-react~~ Upgrade mobx, mobx-react and mobx-state-tree Jan 29, 2021

eatyourgreens mentioned this pull request Jan 29, 2021

Classifier: Fix UPP store tests #2010

Closed

18 tasks

eatyourgreens force-pushed the upgrade-mobx-react branch 2 times, most recently from 2cdf2c2 to 72cf8bd Compare February 1, 2021 10:22

eatyourgreens added 6 commits February 1, 2021 11:04

Upgrade mobx and mobx-react

a777f6b

Update the NextJS apps and the classifier to each use [email protected] and [email protected]

Fix broken collections modal refs

e3c5006

Bump mobx-state-tree to 3.15 across packages

315c896

Bump MST and mobx-react

1a7cf29

Bump mobx-react to 6.3 and mobx-state-tree to 3.17 for the NextJS apps. Upgrading the classifier breaks the tests for the store models, so I've held off on that.

Rewrite UPPStore tests with mobx.when

cd235ac

Upgrade to MST 3.17

7113e7c

eatyourgreens force-pushed the upgrade-mobx-react branch from 2da7a0f to 7113e7c Compare February 1, 2021 11:04

eatyourgreens requested a review from shaunanoordin February 1, 2021 11:31

eatyourgreens assigned shaunanoordin Feb 1, 2021

eatyourgreens commented Feb 1, 2021

View reviewed changes

Tidy up UPP store tests

dd3e4a4

Move duplicated code into a function. Run tests after the UPP has either loaded or errored, then check that the expected state is set.

eatyourgreens force-pushed the upgrade-mobx-react branch from 39e02d6 to dd3e4a4 Compare February 1, 2021 12:59

eatyourgreens removed request for a team and srallen February 1, 2021 13:51

shaunanoordin approved these changes Feb 1, 2021

View reviewed changes

github-actions bot added the approved This PR is approved for merging label Feb 1, 2021

eatyourgreens merged commit 00e9e29 into master Feb 1, 2021

eatyourgreens deleted the upgrade-mobx-react branch February 1, 2021 22:50

srallen mentioned this pull request Feb 3, 2021

mobx-react v.6.1.4 breaks classifications without annotations on the first subject #1839

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upgrade mobx, mobx-react and mobx-state-tree #2009

Upgrade mobx, mobx-react and mobx-state-tree #2009

eatyourgreens commented Jan 28, 2021 •

edited

Loading

eatyourgreens commented Jan 28, 2021 •

edited

Loading

eatyourgreens commented Jan 29, 2021

eatyourgreens commented Jan 29, 2021

eatyourgreens commented Jan 29, 2021 •

edited

Loading

eatyourgreens Feb 1, 2021

shaunanoordin left a comment

shaunanoordin commented Feb 1, 2021

Upgrade mobx, mobx-react and mobx-state-tree #2009

Upgrade mobx, mobx-react and mobx-state-tree #2009

Conversation

eatyourgreens commented Jan 28, 2021 • edited Loading

Review Checklist

General

Components

Apps

Publishing

Post-merging

eatyourgreens commented Jan 28, 2021 • edited Loading

eatyourgreens commented Jan 29, 2021

eatyourgreens commented Jan 29, 2021

eatyourgreens commented Jan 29, 2021 • edited Loading

eatyourgreens Feb 1, 2021

Choose a reason for hiding this comment

shaunanoordin left a comment

Choose a reason for hiding this comment

PR Review

Testing

Testing Limitations

Status

shaunanoordin commented Feb 1, 2021

eatyourgreens commented Jan 28, 2021 •

edited

Loading

eatyourgreens commented Jan 28, 2021 •

edited

Loading

eatyourgreens commented Jan 29, 2021 •

edited

Loading