Project

General

Profile

Actions

Bug #17782

closed

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

Added by Lucas Di Pentima almost 3 years ago. Updated over 2 years ago.

Status:
Resolved
Priority:
Normal
Assigned To:
Category:
Workbench2
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 1 (0 open1 closed)

Arvados - Task #17790: Review 17782-react-scripts-ts-migrationResolvedNico César07/06/2021Actions
Actions #1

Updated by Peter Amstutz almost 3 years ago

  • Assigned To set to Lucas Di Pentima
Actions #2

Updated by Lucas Di Pentima almost 3 years ago

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

Updated by Lucas Di Pentima almost 3 years ago

  • Status changed from New to In Progress
Actions #4

Updated by Lucas Di Pentima over 2 years ago

Updates at arvados-workbench2|e2914bad - branch 17782-react-scripts-ts-migration
Test run: 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.

Actions #5

Updated by Lucas Di Pentima over 2 years ago

  • Target version changed from 2021-07-07 sprint to 2021-07-21 sprint
Actions #6

Updated by Lucas Di Pentima over 2 years 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: developer-tests-workbench2: #445

Actions #7

Updated by Lucas Di Pentima over 2 years ago

Running arvados tests with nodejs v12 from arvados-server install updates: developer-run-tests: #2574
Successful wb1 integration tests run: developer-run-tests-apps-workbench-integration: #2731

Actions #8

Updated by Nico César over 2 years 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)

Actions #9

Updated by Lucas Di Pentima over 2 years ago

Merged 17782-nodejs-update at commit:b59aca2b57068547827d7b681670fb197ad0e144

Actions #10

Updated by Nico César over 2 years 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.

Actions #11

Updated by Lucas Di Pentima over 2 years ago

Updates at arvados-workbench2|c32095f0
Test run: 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.

Actions #12

Updated by Lucas Di Pentima over 2 years ago

Merged main with their corresponding fixes at arvados-workbench2|38d27e9
Test run: developer-tests-workbench2: #447

Actions #13

Updated by Nico César over 2 years ago

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

LGTM! ready to merge

Actions #14

Updated by Lucas Di Pentima over 2 years ago

Thanks!

Actions #15

Updated by Lucas Di Pentima over 2 years ago

  • Status changed from In Progress to Resolved
Actions #16

Updated by Lucas Di Pentima over 2 years ago

  • Release set to 41
Actions

Also available in: Atom PDF