Bug #17782

Migrate WB2 app from react-scripts-ts to create-react-app

Added by Lucas Di Pentima 4 months ago. Updated about 2 months ago.

Status:
Resolved
Priority:
Normal
Assigned To:
Category:
Workbench2
Start date:
07/06/2021
Due date:
% Done:

100%

Estimated time:
(Total: 0.00 h)
Story points:
-
Release relationship:
Auto

Description

Currently we have lots of old packages that cannot (or shouldn't) be upgraded because they're indirect dependencies of the abandoned react-scripts-ts package.
For some time now, the create-react-app package supports Typescripts so it should be able to upgrade the app to it, unlocking further security package upgrades.
Also, we're probably missing on the newer testing/linting features because of this.

Here's one guide on how to do the migration: https://vincenttunru.com/migrate-create-react-app-typescript-to-create-react-app/


Subtasks

Arvados - Task #17790: Review 17782-react-scripts-ts-migrationResolvedNico César

Associated revisions

Revision ebe111f7 (diff)
Added by Ward Vandewege 3 months ago

Allow building a wb2 jenkins image from a specific wb2 git commit.

refs #17782

Arvados-DCO-1.1-Signed-off-by: Ward Vandewege <>

Revision b59aca2b
Added by Lucas Di Pentima 3 months ago

Merge branch '17782-nodejs-update' into main
Refs #17782

Arvados-DCO-1.1-Signed-off-by: Lucas Di Pentima <>

Revision b6ac7fe8
Added by Lucas Di Pentima 3 months ago

Merge branch '17782-react-scripts-ts-migration' into main. Closes #17782

Arvados-DCO-1.1-Signed-off-by: Lucas Di Pentima <>

Revision 0b6efc11 (diff)
Added by Peter Amstutz 3 months ago

arvbox uses react-scripts, refs #17782

Arvados-DCO-1.1-Signed-off-by: Peter Amstutz <>

Revision e10f8abe (diff)
Added by Peter Amstutz 3 months ago

arvbox uses react-scripts, refs #17782

Arvados-DCO-1.1-Signed-off-by: Peter Amstutz <>

History

#1 Updated by Peter Amstutz 4 months ago

  • Assigned To set to Lucas Di Pentima

#2 Updated by Lucas Di Pentima 4 months ago

  • Target version changed from 2021-06-23 sprint to 2021-07-07 sprint

#3 Updated by Lucas Di Pentima 4 months ago

  • Status changed from New to In Progress

#4 Updated by Lucas Di Pentima 3 months ago

Updates at arvados-workbench2|e2914bad - branch 17782-react-scripts-ts-migration
Test run: https://ci.arvados.org/view/Developer/job/developer-tests-workbench2/442

Updates

  • Migrates from react-scripts-ts to react-scripts 3.4.4
  • Fixes some cypress tests.
  • Changes node requirement to be version 12. (CI pending to be updated to be able to test this on the development pipeline)
  • Updates forced package upgrades (from resolutions key in packages.json) to let the canonical deps to correctly be resolved:
    • yarn audit on main branch: Severity: 100 Low | 45 Moderate | 1 High (from 1928 packages)
    • yarn audit on this branch: Severity: 4 Low | 5 Moderate | 1 High (from 2073 packages)

Notes

Lots of changes in linter rule strictness, TS compilation, and other environmental features required major changes in the code base, updating module imports, type-related code changes, and other tasks.
I recommend re-creating the node_modules/ directory from scratch as I have seen issues when doing an upgrade from a previous state.
I tried to go from react-scripts 3.4.4 to 4.0.0 but the number of errors produced talked me out of that, we can go into that adventure in the future.

#5 Updated by Lucas Di Pentima 3 months ago

  • Target version changed from 2021-07-07 sprint to 2021-07-21 sprint

#6 Updated by Lucas Di Pentima 3 months ago

Updates at commit:9d7aa0bee - branch 17782-nodejs-update (arvados repo)

  • Changes arvados-server install's nodejs requirements to be version 12.

Updates at arvados-workbench2|43fff13 (wb2 branch)

  • Adds an arvados-server install run as a prerequisite to run yarn install on the Makefile, so the correct nodejs version is available.

Now Jenkins is able to run the tests: https://ci.arvados.org/view/Developer/job/developer-tests-workbench2/445

#7 Updated by Lucas Di Pentima 3 months ago

Running arvados tests with nodejs v12 from arvados-server install updates: https://ci.arvados.org/job/developer-run-tests/2574/
Successful wb1 integration tests run: https://ci.arvados.org/job/developer-run-tests-apps-workbench-integration/2731/

#8 Updated by Nico César 3 months ago

I think we should merge commit:9d7aa0bee - branch 17782-nodejs-update (arvados repo)

make workbech2-build-image does:

Step 7/7 : RUN git clone https://git.arvados.org/arvados.git && cd arvados &&     go mod download &&     go run ./cmd/arvados-server install -type test && cd .. &&     rm -rf arvados &&     apt-get clean
 ---> Running in 6087c482804d
Cloning into 'arvados'...

also noticed that you need to add git to the docker/Dockerfile if you change to debian:buster (this is not included)

#9 Updated by Lucas Di Pentima 3 months ago

Merged 17782-nodejs-update at commit:b59aca2b57068547827d7b681670fb197ad0e144

#10 Updated by Nico César 3 months ago

I tried the Docker image and git was missing, once I added it to apt-get there was an issue with yarn. This could be the case that /var/lib/arvados/node-v12.22.2-linux-x64/bin/yarn is not in the PATH, here is my output

docker run -ti -v$PWD:$PWD -p 3000:3000 -w$PWD workbench2-build /bin/bash
root@3e74c0c675f6:/home/nico/jobs/curii/arvados-workbench2# npm install --global yarn> yarn@1.22.10 preinstall /var/lib/arvados/node-v12.22.2-linux-x64/lib/node_modules/yarn
> :; (node ./preinstall.js > /dev/null 2>&1 || true)/var/lib/arvados/node-v12.22.2-linux-x64/bin/yarn -> /var/lib/arvados/node-v12.22.2-linux-x64/lib/node_modules/yarn/bin/yarn.js
/var/lib/arvados/node-v12.22.2-linux-x64/bin/yarnpkg -> /var/lib/arvados/node-v12.22.2-linux-x64/lib/node_modules/yarn/bin/yarn.js
+ yarn@1.22.10
added 1 package in 1.325s
root@3e74c0c675f6:/home/nico/jobs/curii/arvados-workbench2# yarn install
bash: yarn: command not found
root@3e74c0c675f6:/home/nico/jobs/curii/arvados-workbench2# /var/lib/arvados/node-v12.22.2-linux-x64/bin/yarn
yarn install v1.22.10
[1/4] Resolving packages...
warning Resolution field "css-what@5.0.1" is incompatible with requested version "css-what@^3.2.1" 

after that it compiled with the following errors:

Compiled with warnings.

./src/lib/cwl-svg/graph/edge.ts
  Line 13:16:  'sourceSide' is assigned a value but never used  @typescript-eslint/no-unused-vars
  Line 14:16:  'destSide' is assigned a value but never used    @typescript-eslint/no-unused-vars
  Line 68:16:  'sourceSide' is assigned a value but never used  @typescript-eslint/no-unused-vars
  Line 69:16:  'destSide' is assigned a value but never used    @typescript-eslint/no-unused-vars

./src/lib/cwl-svg/utils/html-utils.ts
  Line 13:46:  Unnecessary escape character: \/  no-useless-escape

./src/lib/cwl-svg/plugins/selection/selection.ts
  Line 149:13:  Expected a conditional expression and instead saw an assignment  no-cond-assign
  Line 154:20:  Expected a conditional expression and instead saw an assignment  no-cond-assign

./src/lib/cwl-svg/graph/workflow.ts
  Line 194:17:  'arrangeNecessary' is assigned a value but never used  @typescript-eslint/no-unused-vars

./src/lib/cwl-svg/utils/geometry.ts
  Line 19:36:  Use 'as SVGGElement' instead of '<SVGGElement>'  @typescript-eslint/consistent-type-assertions

./src/services/collection-service/collection-service-files-response.ts
  Line 23:41:  Unnecessary escape character: \/  no-useless-escape

./src/store/dialog/with-dialog.ts
  Line 23:9:  'P' is defined but never used  @typescript-eslint/no-unused-vars

./src/lib/cwl-svg/plugins/port-drag/port-drag.ts
  Line 268:29:  Use 'as any' instead of '<any>'  @typescript-eslint/consistent-type-assertions

./src/views/workbench/workbench-loading-screen.tsx
  Line 29:9:  img elements must have an alt prop, either with meaningful text, or an empty string for decorative images  jsx-a11y/alt-text

./src/lib/cwl-svg/plugins/deletion/deletion.ts
  Line 32:28:  Unexpected mix of '&&' and '||'  no-mixed-operators
  Line 32:47:  Unexpected mix of '&&' and '||'  no-mixed-operators

./src/lib/cwl-svg/utils/dom-events.ts
  Line 34:39:  Function declared in a loop contains unsafe references to variable(s) 'target'  no-loop-func

./src/lib/cwl-svg/plugins/edge-hover/edge-hover.ts
  Line 43:12:  The function binding is unnecessary  no-extra-bind
  Line 49:12:  The function binding is unnecessary  no-extra-bind

./src/views/workbench/fed-login.tsx
  Line 45:32:  <iframe> elements must have a unique title property  jsx-a11y/iframe-has-title

./src/common/url.ts
  Line 6:37:  Unnecessary escape character: \[  no-useless-escape

./src/components/tree/virtual-tree.tsx
  Line 87:25:   '_' is defined but never used  @typescript-eslint/no-unused-vars
  Line 177:32:  '_' is defined but never used  @typescript-eslint/no-unused-vars

Search for the keywords to learn more about each warning.
To ignore, add // eslint-disable-next-line to the line before.

tested with the following public/config.json

{
  "API_HOST": "ce8i5.arvadosapi.com",
  "FILE_VIEWERS_CONFIG_URL": "file-viewers-example.json" 
}

and all this seems to be working.

#11 Updated by Lucas Di Pentima 3 months ago

Updates at arvados-workbench2|c32095f0
Test run: https://ci.arvados.org/view/Developer/job/developer-tests-workbench2/446/

  • Changes back the docker base image from debian:buster to node:12.22.3-buster as it also installs other needed packages like yarn
  • Fixes the majority of the linter warnings, there're 2 or 3 cases where I just asked the linter to just shut up because I'm not sure what's the correct solution.

Couldn't test the docker changes yet as I'm having issues with my home connection and it never passes the git clone stage, will retry tomorrow.

#13 Updated by Nico César 3 months ago

tested docker and standalone commit:38d27e9|arvados-workbench2

LGTM! ready to merge

#14 Updated by Lucas Di Pentima 3 months ago

Thanks!

#15 Updated by Lucas Di Pentima 3 months ago

  • Status changed from In Progress to Resolved

#16 Updated by Lucas Di Pentima about 2 months ago

  • Release set to 41

Also available in: Atom PDF