Fix Node.js 18 compatibility, upgrade to create-react-app v5 and make mac build universal

[ClementPasteau]

Follow-up of #5269 to test the CI

[ClementPasteau]

@jtsorlinis create this PR to test the CI.
If you need to bring any change, feel free to push on your first branch and I will cherry-pick the commit here

[4ian]

• Universal DMG is here.
• latest-mac.yml is here

I hope auto update will work.

[4ian]

Seein this issue on my mac intel:

Uncaught Exception:
TypeError: Mime is not a constructor
at Object.<anonymous> (/Applications/GDevelop 5.app/Contents/Resources/app.asar/node_modules/mime/index.js:4:18)
at Module._compile (node:internal/modules/cjs/loader:1116:14)
at Object.Module._extensions..js (node:internal/modules/cjs/loader:1169:10)
at Module.load (node:internal/modules/cjs/loader:988:32)
at Module._load (node:internal/modules/cjs/loader:829:12)
at Function.c._load (node:electron/js2c/asar_bundle:5:13343)
at Module.require (node:internal/modules/cjs/loader:1012:19)
at require (node:internal/modules/cjs/helpers:102:18)
at Object.<anonymous> (/Applications/GDevelop 5.app/Contents/Resources/app.asar/node_modules/live-server/node_modules/send/index.js:24:12)
at Module._compile (node:internal/modules/cjs/loader:1116:14)

Maybe related, in the terminal:

(node:3978) [DEP0128] DeprecationWarning: Invalid 'main' field in '/Applications/GDevelop 5.app/Contents/Resources/app.asar/node_modules/mime/package.json' of 'mime.js'. Please either fix that or report it to the module author
(Use `GDevelop 5 --trace-deprecation ...` to show where the warning was created)

Strange because we've not changed Electron version or packages? Also looking at the yarn.lock/package-lock.json, seems like the mime module was not changed.

[4ian]

@jtsorlinis Any chance you can try this build (link in my previous posts)? Seems like the CI made something that is not working somehow.
What was the process/commands you ran to make your own build? :)

[jtsorlinis]

@4ian Getting the same error with the above DMG. My local one seems to work fine.

I just realised I did run one command differently, I installed the latest version of emsdk as the 1.39.6 one didn't seem to even download/install for me.

I've added a commit here to my original PR if you want to cherry pick it and see if it fixes the issues. https://github.com/jtsorlinis/GDevelop/commit/7eaeca11e12de19537c6760218882f407ebe79d3

If it does, I can update all the references

[4ian]

I just realised I did run one command differently, I installed the latest version of emsdk as the 1.39.6 one didn't seem to even download/install for me.

In theory this should not impact the editor in this way, as the result of emsdk is a self contained library (libGD.js) used to manipulate game. It's super important but I don't see why compiling it differently would make this kind of error (worst case, the editor would start and error when loading it - but we would see a window).

(btw, did you manager to successfully compile GDevelop.js, i.e: run npm run build in GDevelop.js folder, using this version of emsdk? If so, we should update it and update the CIs to use it. There is a fairly extensive set of tests for it so it hey all pass, i.e: npm run test in GDevelop.js, we're almost certainly good)

[jtsorlinis]

You're correct about the emsdk version not making a difference. I've tried it even without a local emsdk and it seems to work too.

1. Cloned a fresh repo and checkout branch
2. cd newIDE/app && npm install && cd ../electron-app && npm install
3. cd ../..
4. cd newIDE/electron-app && npm run build -- --mac --publish=never
5. Working dmg, zip and .app in electron-app/dist folder

[4ian]

Thanks for confirming! This is then... even stranger!
I tried the Windows build and it seems to work properly, which tends to tell that the issue is not with the package-lock.json.

When you build, can you confirm you're not getting the app notarized? I.e, you're not seeing these first 2 lines:

Can you confirm the output of the npm run build -- --mac ends like this:

(trying to find if there is no a change in a version of something like Electron).

[jtsorlinis]

Yep, no signing on my part.

Only difference I can see is my OS version, and the fact that its using APFS for the dmg.

[jtsorlinis]

It's very strange that the build isn't failing locally as I'm using the same Node version as the pipeline (18).

It seems to be failing due to insecure SSL and it seems like an easy fix could be updating the react-scripts version as mentioned here. https://stackoverflow.com/a/71334532

This also upgrades to webpack 5 which means we needed to manually install 'path' as webpack 5 no longer provides the shims by default.

The build runs successfully locally, but I have no idea if it will help with CircleCI.

I've made a commit on my branch again here: https://github.com/jtsorlinis/GDevelop/commit/3adefbd906ace9cea3ebfc2fcc42665b693eb2a3

[ClementPasteau]

I've pushed the commit

[4ian]

This also upgrades to webpack 5 which means we needed to manually install 'path' as webpack 5 no longer provides the shims by default.

The build runs successfully locally, but I have no idea if it will help with CircleCI.

Even if it does not solve the issue with CircleCI, it would still be a huge win to get this updated and so the app to work with Node.js 18+, so thank you :)
The build is failing because on CI is considering linting warnings as errors. We can either fix them or ignore them to see if things are better.

[jtsorlinis]

Looks like without the linting it worked!

Downloaded the dmg from CircleCI and its working fine for me now 👍

[jtsorlinis]

Just pushed another commit to fix the CI errors
https://github.com/jtsorlinis/GDevelop/commit/43b7b078a0d8216e0bb34dbd6cbaf7510bfcf0f6

[4ian]

Great news that it works :) I've not had a chance yet to test it as I was busy.
Upgrading to react-scripts/CRA v5 needs a careful check that everything works. For example, from what I remember the development version (when you do npm start) was crashing because of a react-error-overlay related issue (facebook/create-react-app#11773).

Does the DMG version from CircleCI seems to run well (preview working, adding an image/sound from a file, saving/loading a project)?
If so, then maybe the issue with react-error-overlay is just what we need to fix to be able to upgrade to CRA 5 and unlock Node.js 18+, which would be a big win :)

Still unsure why CircleCI was unable to make a working universal DMG without this though.

[4ian]

A few more things:

• the "path" module is super outdated and I think not recommened. Instead, we can "simply" change all the from 'path' to from 'path-browserify', which seems to be a more recommended implementation of path for browsers. Needs testing but I think it's safe and fine.
• With Node.js 18, unzipper, which we use to extract zip files of external editors (or libraries like Zip.js) creates corrupted files. I've just had the issue on Windows. See Files extracted from zip are currupt ZJONSSON/node-unzipper#271. We probably need to upgrade (I'm trying) or move to another library.

[jtsorlinis]

npm start is working fine for me.
Haven't done thorough testing but everything I've tried seems to work fine in the build.

If there's anything else I can do to help let me know!

[4ian]

I've done more changes to fix the unzipping at installation in the various scripts we have on Windows on Node 18, and from testing this seems to work. I'll check again this time on mac and hopefully if nothing broken we should be good!

[4ian]

I'm sorry I'm delaying this so much, I'm super busy but we'll hopefully release a new version and then we can go ahead with this :)

[ClementPasteau]

Rebased to fix conflicts of packages

[ClementPasteau]

The build seems to work, but the app doesn't work locally on my machine.

The path package being used seems to be using a variable "process" that does not exist on web.
I'm not sure exactly how to fix that. (patching that package? this is the one used: https://github.com/jinder/path)

I've tried using path-browserify instead, but then it needs to be defined as a polyfill for path in the app, and thus we need to define a plugin in the webpack config to do so. (see https://stackoverflow.com/questions/64557638/how-to-polyfill-node-core-modules-in-webpack-5)

It means we either need to eject or use react-app-rewired or use something like https://github.com/dilanx/craco to avoid ejecting.
I feel like trying craco. Thoughts on that @4ian?

[4ian]

Fixed by doing this:

• the "path" module is super outdated and I think not recommened. Instead, we can "simply" change all the from 'path' to from 'path-browserify', which seems to be a more recommended implementation of path for browsers. Needs testing but I think it's safe and fine.

And replacing unzipper by adm-zip which works well on Windows.

[4ian]

Let's see if the builds are working. Local dev seems to work, despite warnings to fix from webpack 5.

[jtsorlinis]

Sorry I haven't been around. I know a while back you mentioned using the path-browserify package instead of path. Would that fix the path issue?

[4ian]

It seems that it fixed the issue yes :)
I gave a try to the universal macos dmg on a Intel mac, it works well.
The development version also works well on Windows.

I believe what remains to do is:

• Fix the Jest tests somehow failing because of a Node module using "export": https://gdevelop.semaphoreci.com/jobs/8958082b-6da2-491f-ad86-0b3a2cb3ab67
• Check the Windows version works well (need to fix test before, hopefully if the macOS version works, no reasons for Windows to fail).
• Check how we can delete most of the warnings in development.

[ClementPasteau]

• Fixed the tests by ignoring the package with jest.

• For the warnings, I'm not sure we can fix that easily (see After Upgrading to CRA 5.0 getting a lot of: Failed to parse source map from facebook/create-react-app#11767)
  There are 3 packages that are declaring sourcemaps, but that isn't present

  • react-monaco-editor
  • react-mosaic-component
  • qr-creator

  We can try updating those packages to see if it has been fixed with a new version (react-monaco-editor maybe, mosaic-component we have a fork, qr-creator it's the latest version, so we may have to fork it to fix it ourselves, I saw an issue opened on their repo. Or use another library.)
  We could disable completely generating the sourcemaps, with GENERATE_SOURCEMAP=false, but seems overkill.
  Or We try using craco to override webpack config and specify when to ignore warnings (see After Upgrading to CRA 5.0 getting a lot of: Failed to parse source map from facebook/create-react-app#11767 (comment))

I haven't tested windows yet, I can test tomorrow when the build is complete and I have access to one

[4ian]

@ClementPasteau Let's give a try to 8GB and maybe other node options to get this to compile on AppVeyor.

[ClementPasteau]

OK it seems that the memory issue on Windows is fixed by increasing the node memory to 8GB.

I've tested the generated .exe on Windows, all good.
I've tested the generated .dmg on a mac M1, all good.
4ian you've tested the generated .dmg on a mac x86 and no issue raised.

Locally, everything seems to be working on my Mac now.
The only thing remaining would be fixing the warnings about sourcemaps missing.
I think we can go ahead with merging this before too many conflicts arise, and we find a solution for those warnings later.

cc @4ian