Update node to the latest LTS version

[lucapette]

as per discussion in #1243

Unfortunately I can't test this locally so I opened a pull-request before being sure it passes CI (not my "style")

[lucapette]

yes it makes sense it fails :) so we can't do a separate pr for the node-sass. If we upgrade node, we have to upgrade the lib. I will do that now and see if that makes it green)

[lucapette]

Progress with the node sass to sass replacement and the correct version of react test rendered. But apparently not enough.

The error looks funky. I'll investigate

[lucapette]

So unfortunately it seems to me we're going to end up in a very similar place as the other pull requests (as they're all connected in the end).

Updating react scripts will fix the current error (see https://stackoverflow.com/questions/69692842/error-message-error0308010cdigital-envelope-routinesunsupported for more information)

But the build still fails because of the svg images. I opened a separate pull request for that #1252 so when we merge that, I will rebase this branch and we should see the eslint errors only.

We can decide what to do when we get there. Unfortunately the fact that the JS ecosystem moves really fast makes fixing this build a little tricky...

[tchiotludo]

i know it will be tricky one, the svg is merged, you can rebase it if you want

[lucapette]

I just rebased and pushed it :)

Yes it'll be a little tricky but it should improve the situation quite a bit. I'm still quite optimistic we'll get it done 💪.

Let's see what's the next build error now 🙃

[lucapette]

so yes, eslint errors it is. I see two options:

• we fix the errors here (but we'd have to decide what to do with proptypes... as that's probably too much work right now)
• We disable eslint to see if we can merge this now and then do a pr only with eslint fixes.

Not sure what I'd do (I see arguments for both) but surely we should go for whatever you're more comfortable with

[tchiotludo]

maybe make a little test locally to see if the interface is working for now ?
It will be surprising that is working just removing the lint ?
also eslint auto fix will not fix a lot of them (not props but quote one) ?

[lucapette]

Good point! Let me check. I'll report back what I find

[lucapette]

This is so confusing... it runs locally even without disabling eslint. Which means eslint problems are warning for the dev build and errors for the production one. Really funny :)

I'll try to disable eslint so we'll know if we get a green build. I'm not 100% sure honestly

[lucapette]

ah! Almost... I think there's warnings because of the sass upgrade as well. Let me investigate those!

[lucapette]

So I was both right and wrong about sass. Yes there are warnings but those are not the problem. The problem is that react scripts when CI=true will treat warnings as errors (interesting choice!).

Now that it's green, let me recap what I did:

• I update node to latest LTS version.
• I replaced node-sass with sass (there's now some warnings. I can tackle those in their own pr).
• I updated react-test-rendered (because the version was wrong) and react-scripts (because the build would otherwise rely on insecure hashing by default).
• I set CI=false for react-scripts build so this would be green.

I wish we could do these in separate steps but I think it would end up being more work than this. If we merge this, I'll immediately open a draft PR where I remove the CI=false so we can 1) see the eslint errors 2) decide how to tackle them. It will mostly be react props things.

Of course, once again, we should do whatever you're more comfortable with. I understand merging this may be a bit uncomfortable.

[tchiotludo]

Thanks for ✔️ ;)
To be honest, I'm not comfortable merging right now, there is a lot of attention on this issue #1215 and will need to release really soon this one.

The better IMO, will be to created another PR using this branch as base ?
In case the next PR is not finished, I'll able to merge at least this one after the release.

What do you think?

[lucapette]

You mean so that I can start working on the eslint warnings? (Sorry I'm not sure I understand the question).

[tchiotludo]

Yes If you want and have the time!
Since we start of PR from this branch, we could merge the 2 independently and will not need to merge this one before the release of next version.

Do you got it? Does it make sense?

[lucapette]

yes! I'll see what I can do with the eslint warnings :)