Skip to content
This repository has been archived by the owner on Jan 9, 2023. It is now read-only.

Upgrade react-scripts to v4.0.x #2454

Open
jackcmeyer opened this issue Oct 26, 2020 · 5 comments
Open

Upgrade react-scripts to v4.0.x #2454

jackcmeyer opened this issue Oct 26, 2020 · 5 comments
Labels
dependencies Pull requests that update a dependency file help wanted indicates that an issue is open for contributions LOE - large indicates that the level of effort to complete issue is large
Projects
Milestone

Comments

@jackcmeyer
Copy link
Member

React scripts was recently upgraded to v4.0.0. This issue is to track and finish migrating HospitalRun to react-scripts v4.0.x. Dependabot couldn't do this on its own due to a failing build: #2437.

Things to consider:

  • npm start should start the app
  • npm test should successfully run the test suite
  • npm run lint should continue to report linting errors.
@jackcmeyer jackcmeyer added help wanted indicates that an issue is open for contributions dependencies Pull requests that update a dependency file LOE - large indicates that the level of effort to complete issue is large labels Oct 26, 2020
@jackcmeyer jackcmeyer added this to the v2.0 milestone Oct 26, 2020
@jackcmeyer jackcmeyer added this to To do in Version 2.0 via automation Oct 26, 2020
@SamuelQZQ
Copy link
Contributor

SamuelQZQ commented Dec 17, 2020

@jackcmeyer I'm new here, let me have a try.

I found that the react-script v4.0x requires jest v26.6.0. But it seems we have failed in upgrade jest a few times. Do you have any information about this?

@doubleppereira
Copy link

Actually react-script v4.0 has jest already you don't need to have that dependency anymore..I was taking a look as well and it seems that after the update I found that we would need to ignore some eslint rules in sake of typescript ones.

'no-shadow': 'off',
    '@typescript-eslint/no-shadow': ['error'],
    // note you must disable the base rule as it can report incorrect errors
    'no-use-before-define': 'off',
    '@typescript-eslint/no-use-before-define': ['error'],

Even like this some tests are failing maybe some other rules or even the tests them selves I notice per example that ViewLabs.test looked like it had some race condition going on there but didn't have much time to investigate

@nobrayner
Copy link
Contributor

nobrayner commented Dec 18, 2020

I've started working on this after trying it out on the Convert to RTL branch.

Currently, I am having a lot of issues with the tests, and I don't seem to know enough to reason about them. Namely a lot of:

TypeError: Right-hand side of 'instanceof' is not callable

      at new ReadableState (node_modules/sublevel-pouchdb/node_modules/readable-stream/lib/_stream_readable.js:105:14)
      at ReadStream.Readable (node_modules/sublevel-pouchdb/node_modules/readable-stream/lib/_stream_readable.js:139:25)
      at new ReadStream (node_modules/sublevel-pouchdb/lib/index.js:289:12)
      at ReadStream (node_modules/sublevel-pouchdb/lib/index.js:286:12)
      at EventEmitter.emitter.readStream.emitter.createReadStream (node_modules/sublevel-pouchdb/lib/index.js:261:14)
      at PouchDB.LevelPouch.api._changes (node_modules/pouchdb-adapter-leveldb-core/lib/index.js:1102:42)
      at Changes$1.Object.<anonymous>.Changes$1.doChanges (node_modules/pouchdb/lib/index.js:1569:28)
      at node_modules/pouchdb/lib/index.js:1508:12
      at Object.validate (node_modules/pouchdb/lib/index.js:3805:3)
      at Changes$1.Object.<anonymous>.Changes$1.validateChanges (node_modules/pouchdb/lib/index.js:1504:34)
      at new Changes$1 (node_modules/pouchdb/lib/index.js:1465:10)
      at PouchDB.Object.<anonymous>.AbstractPouchDB.changes (node_modules/pouchdb/lib/index.js:2326:10)
      at processNextBatch (node_modules/pouchdb-abstract-mapreduce/lib/index.js:610:28)
      at updateViewInQueue (node_modules/pouchdb-abstract-mapreduce/lib/index.js:672:12)
      at node_modules/pouchdb-abstract-mapreduce/lib/index.js:578:14
      at node_modules/pouchdb-mapreduce-utils/lib/index.js:92:29
      at node_modules/pouchdb-abstract-mapreduce/lib/index.js:25:12

And then the real kicker, that completely kills the test run:

src/__tests__/patients/view/ViewPatient.test.tsx
C:\_Source\hospitalrun-frontend-nobrayner\node_modules\react-dom\cjs\react-dom.development.js:154
      var evt = document.createEvent('Event'); // Keeps track of whether the user-provided callback threw an error. We
                         ^

TypeError: Cannot read property 'createEvent' of null
    at Object.invokeGuardedCallbackDev (C:\_Source\hospitalrun-frontend-nobrayner\node_modules\react-dom\cjs\react-dom.development.js:154:26)
    at invokeGuardedCallback (C:\_Source\hospitalrun-frontend-nobrayner\node_modules\react-dom\cjs\react-dom.development.js:292:31)
    at flushPassiveEffectsImpl (C:\_Source\hospitalrun-frontend-nobrayner\node_modules\react-dom\cjs\react-dom.development.js:22853:9)
    at unstable_runWithPriority (C:\_Source\hospitalrun-frontend-nobrayner\node_modules\scheduler\cjs\scheduler.development.js:653:12)
    at runWithPriority$1 (C:\_Source\hospitalrun-frontend-nobrayner\node_modules\react-dom\cjs\react-dom.development.js:11039:10)
    at flushPassiveEffects (C:\_Source\hospitalrun-frontend-nobrayner\node_modules\react-dom\cjs\react-dom.development.js:22820:12)
    at Object.<anonymous>.flushWork (C:\_Source\hospitalrun-frontend-nobrayner\node_modules\react-dom\cjs\react-dom-test-utils.development.js:876:10)
    at Immediate.<anonymous> (C:\_Source\hospitalrun-frontend-nobrayner\node_modules\react-dom\cjs\react-dom-test-utils.development.js:887:11)

EDIT: Got some help (@mpeyper) and it seems to be related to tests hovering around the 5s mark for test running time. jest.setTimeout(10000) resolves in this case. Hopefully this can be removed as part of the conversion to RTL #2516

@nobrayner
Copy link
Contributor

I may revisit this after the RTL conversion, to see if it makes a difference. At the moment, 80+ tests fail for various reasons. And the number changes each time all the tests are run...

@doubleppereira
Copy link

I may revisit this after the RTL conversion, to see if it makes a difference. At the moment, 80+ tests fail for various reasons. And the number changes each time all the tests are run...

Yes I think the way the tests are being tested might have some race conditions happening so maybe with that conversion to RTL would solve that and would make upgrade to react 17 easier as well. I managed to have around 46 tests failing but I still have those errors that you mentioned above

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
dependencies Pull requests that update a dependency file help wanted indicates that an issue is open for contributions LOE - large indicates that the level of effort to complete issue is large
Projects
Version 2.0
  
To do
Development

No branches or pull requests

4 participants