upgrade react-scripts to latest version 5.0.1

[erasta]

currently react-scripts version is 2.x when upgrading react-scripts several packages and components break, graphql included. please upgrade react-scripts so we can advance to modern maps and material ui.

[shimlinnovate]

@sarareisner יכולה בבקשה להתייחס לבקשה מבחינת משמעות של כמות עבודה בבקשה

[erasta]

@shimlinnovate @yehudarav any update on this?

[alex-bondarev-linnovate]

react-scripts is bundled into CRA, which is no longer recommended as a way to quickly start a new React App. Have you considered shifting to alternatives instead?

In the official docs they recommend going with Vite as a bundler if one does not want a framework.

[alex-bondarev-linnovate]

There's been a lot of discussion about the future of Create React App recently. There's a ~2-month-old commend by Dan (part of the React team) about the future of CRA and there's a very recent Twitter thread too. TLDR of those links is - CRA will be changed into something else.

So, to complete this task we'll need to try and predict the future of this project, let me highlight possible routes:

1. This app will need to be changed from client-side-only to be hybrid (Server-Side Rendering + client interactivity)

If we see the potential of some parts of the app to be prerendered on the server and produce HTML for a faster initial feedback to the user - we'd need to migrate the app to NextJS or Remix.

2. This app will remain client-side only

If we don't see a need for SSR, we'll have 2 options, each has pluses and minuses:

Option 1. Do what the task is about - update react-scripts to v5.

Drawbacks:

• seems to break old apollo client

As a result - apollo client will need to be updated too. Unfortunately it currently conflicts with Material UI, npm cannot create dependency tree if we update react-scripts, apollo client, but not Material. It seems that all 3 parts will need to be updated simultaneously - react-scripts, apollo client and material UI, which complicates things.

• updating react-scripts is not future-proof

One of the last updates in its current form, the future of react-scripts is super unclear, it's highly likely that the project will be discontinued/changed/transformed/abandoned, so we'll still need to migrate from or adapt it anyways when such changes occur. See links above for more info.

Benefits:

I see no benefits. The update is not easy and has breaking changes.

Option 2. Migrate to alternative tech - Vite

Drawbacks:

• will require components change, like renaming of files from .js to .jsx
• absolute paths will need to be handled a bit differently. Vite provides configuration for it.
• all components will need to be rechecked in both versions - dev and production, because different mechanisms are used for Dev and Build (can provide more details if needed)

Benefits:

• officially recommended as a frameworkless approach in the React documentation
• relatively easy migration of most of the codebase
• future-proof, on-going project with an active community

To recap, my recommendations would be to:

a) decide if we need SSR (if yes then migrate to NextJS), b) if no, then migrate to Vite as a future-proof alternative of react-scripts.

I need you decision on this.

[yehudarav]

Hi. How long will option 1 and 2.2 will take?

On Mon, 20 Mar 2023, 10:34 am alex-bondarev-linnovate, < @.***> wrote:

There's been a lot of discussion about the future of Create React App recently. There's a ~2-month-old commend by Dan https://github.com/reactjs/react.dev/pull/5487#issuecomment-1409720741 (part of the React team) about the future of CRA and there's a very recent Twitter thread https://twitter.com/dan_abramov/status/1636827365677383700 too. TLDR of those links is - CRA will be changed into something else.

So, to complete this task we'll need to try and predict the future of this project, let me highlight possible routes:

1. This app will need to be changed from client-side-only to be hybrid (Server-Side Rendering + client interactivity)

If we see the potential of some parts of the app to be prerendered on the server and produce HTML for a faster initial feedback to the user - we'd need to migrate the app to NextJS or Remix.

2. This app will remain client-side only

If we don't see a need for SSR, we'll have 2 options, each has pluses and minuses:

Option 1. Do what the task is about - update react-scripts to v5.

Drawbacks:

• seems to break old apollo client

As a result - apollo client will need to be updated too. Unfortunately it currently conflicts with Material UI, npm cannot create dependency tree if we update react-scripts, apollo client, but not Material. It seems that all 3 parts will need to be updated simultaneously - react-scripts, apollo client and material UI, which complicates things.

• updating react-scripts is not future-proof

One of the last updates in its current form, the future of react-scripts is super unclear, it's highly likely that the project will be discontinued/changed/transformed/abandoned, so we'll still need to migrate from or adapt it anyways when such changes occur. See links above for more info.

Benefits:

I see no benefits. The update is not easy and has breaking changes.

Option 2. Migrate to alternative tech - Vite https://main.vitejs.dev/guide/

Drawbacks:

• will require components change, like renaming of files from .js to .jsx
• absolute paths will need to be handled a bit differently. Vite provides configuration for it.
• all components will need to be rechecked in both versions - dev and production, because different mechanisms are used for Dev and Build (can provide more details if needed)

Benefits:

• officially recommended as a frameworkless approach in the React documentation https://react.dev/learn/start-a-new-react-project
• relatively easy migration of most of the codebase
• future-proof, on-going project with an active community

To recap, my recommendations would be to:

a) decide if we need SSR (if yes then migrate to NextJS), b) if no, then migrate to Vite as a future-proof alternative of react-scripts.

I need you decision on this.

— Reply to this email directly, view it on GitHub https://github.com/argosp/trialdash/issues/331#issuecomment-1475815556, or unsubscribe https://github.com/notifications/unsubscribe-auth/AMCON2KXDB35XFXBU4RJEH3W5AJBFANCNFSM6AAAAAAVKR7Z7E . You are receiving this because you were mentioned.Message ID: @.***>

[alex-bondarev-linnovate]

Option 1 (moving to NextJS) is not just just about the time it takes to migrate, it's also about developers writing code differently. NextJS is a different framework, yes - it's React-based - but it's a framework, so any existing solutions (e.g. routes) will need to be transferred, new folder structure will be created, etc. I don't have an estimate for that simply because I don't know the project that well. There's an official guide on how to migrate which is handy.

We need to move to NextJS not as an alternative to react-scripts, but as an alternative of having a client-side app. NextJS is needed if we plan to have server-side rendering (SSR), like for search engine optimisation, for example, or for writing an API for our client code. If these reasons are not something that you want, then we shouldn't look at NextJS.

Option 2 (moving to Vite). Little changes to folder structure and general coding, it'll still be a client-side React app.

I managed to start the application locally relatively quickly (it took me ~1.5 day to research the needed changes). I didn't test all of the components though, e.g. I couldn't load the map of the entities page (it was erroring), but in general the app looked more or less OK (login screen worked, tables looked OK, routing was working).

We can create a separate Git branch for the update where we might see the scale for component changes. Depending on that scale we can make a decision. From my perspective - I'd rather update app components than research on why apollo breaks with the latest react-scripts :)

To recap - to spin up a local Vite app will be relatively easy, I've done most of the changes already and can run a copy of the app locally. All that will be needed will be to test building of the app and opening critical functionality (like a map with tests which didn't open for me) and fix it if it needs fixing.

[shimlinnovate]

@yehudarav Hi Yehuda please read @alex-bondarev-linnovate Research regarding @erasta upgrade request, we need to decide on a path to continue the process

[yehudarav]

@Eran Geva @.***> What do you think?

On Tue, Mar 21, 2023 at 7:42 AM shimlinnovate @.***> wrote:

@yehudarav https://github.com/yehudarav Hi Yehuda please read @alex-bondarev-linnovate https://github.com/alex-bondarev-linnovate Research regarding @erasta https://github.com/erasta upgrade request, we need to decide on a path to continue the process

— Reply to this email directly, view it on GitHub https://github.com/argosp/trialdash/issues/331#issuecomment-1477315044, or unsubscribe https://github.com/notifications/unsubscribe-auth/AMCON2KIMYLYKARNX7NC3OLW5E5T7ANCNFSM6AAAAAAVKR7Z7E . You are receiving this because you were mentioned.Message ID: @.***>

[erasta]

Hi @alex-bondarev-linnovate thank you for a great review of the different directions our react app could take.

I believe that a revolutionary change to SSR like NextJs might be problematic here, as it will probably need a full rewrite and take a long time to get a minimal system working. According to @yehudarav the next phase of experiments is needed soon.

So staying with an evolutionary upgrade of the client side react app seems like the right way. As I see it Apollo and MaterialUI needs to be upgraded anyways, either with react-scripts or with Vite.

If you managed to recreate the application with Vite and see the correct experiments and trials, a.k.a the Graphql DB connection working and all, this sounds like a great advancement! The map not working in that configuration is expected, as we are currently using an old react-leaflet package, which we are forced to because of react-script (upgrading it is the incentive of this move).

Please publish this branch with the operable tables views, and i'll branch from it to tweak the map into working.

Many thanks!

[alex-bondarev-linnovate]

@erasta can you help with the following?

ERROR: Permission to argosp/argos.git denied to alex-bondarev-linnovate.
fatal: Could not read from remote repository.

[erasta]

@alex-bondarev-linnovate i think this works only in https, at least for me, try to clone like in https://github.com/argosp/argos#readme

git clone --recursive https://github.com/argosp/argos.git

[alex-bondarev-linnovate]

Can't I push from /trialdash/ to the remote branch?

[erasta]

Did you manage to read from the repo but not to write? There might be a permissions problem. @yehudarav @shimlinnovate @batelkarmi

[alex-bondarev-linnovate]

Both fails for me - https and git. Let me know if I need to correct what I do:

• I recursively cloned the argos project using SSH key (not via https) - it cloned 2 submodules
• I created a feature branch in the ./trialdash/ submodule (not in the parent argos project)
• I cd to ./trialdash and try to push to the remote feature branch

Does it sound sensible?

[erasta]

Cloning should work for you but since it doesn't there might be some permissions missing. I do not have permissions to give you permissions. maybe @shimlinnovate?

Be aware that there are trialdash/Dockerfile and utils/dockers/trialdash/Dockerfile files, one for prod and one for dev, so you'll need to change also the argosp repo.

[erasta]

update: @yehudarav is giving you more permissions now.

[alex-bondarev-linnovate]

I received an invitation, but not to the argos repo :) Will leave that untouched

[alex-bondarev-linnovate]

Was I correct in creating a feature branch in the submodule or should I have created a git branch in the main (argos) repo?

[yehudarav]

I invited you...

On Tue, 21 Mar 2023, 5:46 pm alex-bondarev-linnovate, < @.***> wrote:

I received an invitation, but not to the argos repo :) Will leave that untouched

— Reply to this email directly, view it on GitHub https://github.com/argosp/trialdash/issues/331#issuecomment-1478077176, or unsubscribe https://github.com/notifications/unsubscribe-auth/AMCON2MCXXVTKRYDTM4DPJDW5HENPANCNFSM6AAAAAAVKR7Z7E . You are receiving this because you were mentioned.Message ID: @.***>

[alex-bondarev-linnovate]

@yehudarav I've received an invitation to pyargos, not to regular argos

[yehudarav]

I send another invitation

On Tue, 21 Mar 2023, 5:54 pm alex-bondarev-linnovate, < @.***> wrote:

@yehudarav https://github.com/yehudarav I've received an invitation to pyargos https://github.com/argosp/pyargos, not to regular argos

— Reply to this email directly, view it on GitHub https://github.com/argosp/trialdash/issues/331#issuecomment-1478094419, or unsubscribe https://github.com/notifications/unsubscribe-auth/AMCON2M4D4BK7EMXYYIE2ETW5HFNDANCNFSM6AAAAAAVKR7Z7E . You are receiving this because you were mentioned.Message ID: @.***>

[alex-bondarev-linnovate]

Sorry, the problem was with Password authentication. We'll need to reconfigure submodules to not pull via https. Once I switched the remote url from https to Git, it allowed me to push to a branch - here it is https://github.com/argosp/trialdash/tree/AB/vite

[alex-bondarev-linnovate]

Alas, I overwrote the master changes so it seems ( this branch reapplies the changes made after I pulled it, I'll need to recreate the changes and apply them onto latest master. I was a bit naive to think that I can just copy src from my vite one. I'll need to redo the branch @erasta

[alex-bondarev-linnovate]

@erasta @yehudarav @shimlinnovate I've just repushed all of the changes that need to be made when migrating to Vite, it's 1 commit ahead of the latest master, so contains all of your work.

You can browse this commit and see what changes I made (they are not quite big).

Re: leaflet update. Unfortunately, the components need to be changed if you wish to update to newest leaflet. Upon changing the version locally it installs (no npm conflicts), but requires component updates, which is expected I guess. I'm seeing the following in the console.

does not provide an export named 'Map' (MapWithImage.jsx)

[yehudarav]

So do you have an estimate of the time it takes to perform these updates?

On Thu, Mar 23, 2023 at 8:51 AM alex-bondarev-linnovate < @.***> wrote:

@erasta https://github.com/erasta @yehudarav https://github.com/yehudarav @shimlinnovate https://github.com/shimlinnovate I've just repushed all of the changes that need to be made when migrating to Vite, it's 1 commit ahead of the latest master, so contains all of your work.

You can browse this commit and see what changes I made (they are not quite big).

[image: image] https://user-images.githubusercontent.com/127842115/227124135-2f1d9f81-1d11-473b-be2e-80e3bf63c1d1.png

Re: leaflet update. Unfortunately, the components need to be changed if you wish to update to newest leaflet. Upon changing the version locally it installs (no npm conflicts), but requires component updates, which is expected I guess. I'm seeing the following in the console.

does not provide an export named 'Map' (MapWithImage.jsx)

— Reply to this email directly, view it on GitHub https://github.com/argosp/trialdash/issues/331#issuecomment-1480684547, or unsubscribe https://github.com/notifications/unsubscribe-auth/AMCON2IH47QY3H5AUTHUOALW5PXF7ANCNFSM6AAAAAAVKR7Z7E . You are receiving this because you were mentioned.Message ID: @.***>

[alex-bondarev-linnovate]

@yehudarav I believe that my work has been already done, so no estimations for changing Leaflet or Material UI - those have not been assigned to me AFAIK. @erasta are you taking it from here? You may want to remove the node_modules from the container and delete existing containers before recreating them via docker compose up, just to be sure that the node_modules are coming from the latest package.json.

[erasta]

@alex-bondarev-linnovate Looks good! Everything is working ok on the dev environment correctly, including the hot reload. I will continue the upgrade of react-leaflet if you say there are not NPM conflicts. I'm wondering if the production build of trialdash, that is invoked by CircleCi when merging to master, will also work correctly. Is there an easy way to check that before merging to master? If the merge will go ok, this bug would be closed.

[alex-bondarev-linnovate]

@erasta RE trialdash update. Let me consult my crystal ball - yes, the merge will be without errors :smile: Kidding. Vite provides a way to test it. In package.json you can see the preview script. So vite preview will probably try and build the production build temporarily and maybe open it on the same port, maybe different port, don't know for sure. The build command should also be taken a look at, simply because it uses the env-cmd .env part which may be a requirement for react-script but may not be one for Vite. My recommendation will be to start from there and see how these commands work locally.

[erasta]

@alex-bondarev-linnovate crystal ball is a great method 🤣 i am trying to change https://github.com/argosp/trialdash/blob/master/.circleci/config.yml#L45 to your branch, will see how that works on prod directly. will update, thanks.

[alex-bondarev-linnovate]

OMG, I'd first test locally, just in case @erasta Why straight into production? Sounds so risky to me

[erasta]

Noone actually uses the production at the moment, that is only mock data, they clone and build it on another machine.

[erasta]

nope. changing .circleci/config.yml doesn't change the built branch on prod.

[alex-bondarev-linnovate]

I'd go for changing the docker run commands. By default it runs the dev command, I'd find that place and either a) change it temporarily to 'preview' and see how/if it build, or b) make it a variable, so that local env could start either as development, or as preview. Maybe put it in the .env or something. Just from the top of my head though.

[erasta]

changed the command on docker-compose.yml:L41 to yarn run build && yarn run serve changed the serve command package.json:L53 to serve -s dist as it builds to dist and now all is working locally.

not sure that on production the circleci actually does that, do you know? maybe can you have allocated time to join a screenshare and test this? @alex-bondarev-linnovate FYI @shimlinnovate @yehudarav

[alex-bondarev-linnovate]

@erasta Well done! RE Circle CI - I'm afraid I have little information on the prod environment or on CI, I'm a relatively new guy here. In my opinion - it should be like you describe, we use dev for local development (hot reloading, etc.) and use build for prod (minified and optimised assets, etc.). If it's not set up this way - we'll probably need to find out why, maybe there are reasons. If there are no - I'd go for double-checking and changing the CI config to include the build command