SpacingBat3 / ReForged

A set of :electron: Electron Forge tools, makers and publishers.
https://spacingbat3.github.io/ReForged/
Other
26 stars 7 forks source link

ERR_REQUIRE_ESM #15

Open iameli-streams opened 1 month ago

iameli-streams commented 1 month ago

Hi! Trying to add this to a project that is very nearly the default Electron Forge export and getting ES module errors.

▶ yarn run make
✔ Checking your system
✖ Loading configuration
  › require() of ES Module /home/iameli/code/aquareum/node_modules/@reforged/maker-appimage/dist/main.js from /home/iameli/code/aquareum/js/desktop/forge.config.ts not supported.
    Instead change the require of main.js in /home/iameli/code/aquareum/js/desktop/forge.config.ts to a dynamic import() which is available in all CommonJS modules.
◼ Resolving make targets
◼ Running package command
◼ Running preMake hook
◼ Making distributables
◼ Running postMake hook

Failed to load: /home/iameli/code/aquareum/js/desktop/forge.config.ts

An unhandled rejection has occurred inside Forge:
Error [ERR_REQUIRE_ESM]: require() of ES Module /home/iameli/code/aquareum/node_modules/@reforged/maker-appimage/dist/main.js from /home/iameli/code/aquareum/js/desktop/forge.config.ts not supported.
Instead change the require of main.js in /home/iameli/code/aquareum/js/desktop/forge.config.ts to a dynamic import() which is available in all CommonJS modules.
at require.extensions.<computed> [as .js] (/home/iameli/code/aquareum/node_modules/ts-node/dist/index.js:851:20)
    at Object.<anonymous> (/home/iameli/code/aquareum/js/desktop/forge.config.ts:23:26)
    at m._compile (/home/iameli/code/aquareum/node_modules/ts-node/dist/index.js:857:29)
    at require.extensions.<computed> [as .ts] (/home/iameli/code/aquareum/node_modules/ts-node/dist/index.js:859:16)
    at exports.default (/home/iameli/code/aquareum/node_modules/@electron-forge/core/dist/util/forge-config.js:140:26)
    at async /home/iameli/code/aquareum/node_modules/@electron-forge/core/dist/api/make.js:68:35
    at async _Task.taskFn (/home/iameli/code/aquareum/node_modules/@electron-forge/tracer/dist/index.js:58:20)
    at async _Task.run (/home/iameli/code/aquareum/node_modules/listr2/dist/index.cjs:2063:11)

Repro case is here. I'm not sure why it thinks it's a ES module and none of the rest are. Pretty weird.

iameli-streams commented 1 month ago

Fixed by deleting "type": "module" from maker-appimage's package.json and then everything just worked. Isn't modern JavaScript fun?

SpacingBat3 commented 1 month ago

The makers I offer now there are made for ESM, not CJS. Your config has to be ".mts", else Forge will load stuff as CJS… There's a PR on Forge's side done by me to make ESM supported in similar way to CJS.

SpacingBat3 commented 1 month ago

Also my stance why I dropped ESM without introducing CJS was put there: #14. TL;DR: ESM is at this point supported everywhere (when I started as a web dev, this wasn't really the case and ESM was really problematic with TypeScript and Electron), and since it is Forge that handles module imports, going with ESM-only makers is the right direction for the present/future scenarios.

For other reasons, I'd say having async module loading is another great thing to have, and it fits well the design of the makers themselves.

iameli-streams commented 1 month ago

I hear you, and I don't disagree, ESM is definitely the way to go. I'm just unsure what to do to make it work. forge.config.mts seems to not work for me:

▶ yarn run make
✔ Checking your system
✔ Loading configuration
✖ Resolving make targets
  › Could not find any make targets configured for the "linux" platform.
◼ Running package command
◼ Running preMake hook
◼ Making distributables
◼ Running postMake hook

An unhandled rejection has occurred inside Forge:
Error: Could not find any make targets configured for the "linux" platform.
at /home/iameli/code/aquareum/node_modules/@electron-forge/core/dist/api/make.js:123:27
    at _Task.taskFn (/home/iameli/code/aquareum/node_modules/@electron-forge/tracer/dist/index.js:58:42)
    at _Task.run (/home/iameli/code/aquareum/node_modules/listr2/dist/index.cjs:2063:35)
SpacingBat3 commented 1 month ago

You may check the example on this project, although I think I've built config (AFAIK Forge might need some optdep for TS imports that I didn't install), it seemed to work fine for me at the time of creating this. At worst, I think you can still use CJS, use dynamic imports to get the maker class and try to return a function, I think I saw Forge supporting some syntax with the config as a function when dealing with their config types but I'm not sure entirely on that.

SpacingBat3 commented 1 month ago

As of the said PR, Forge's already merged a good enough support for ESM here: https://github.com/electron/forge/commit/cc0d7d62685fee7d9149915799945f040533fceb

:tada: Hope to see it in action!

coredev-uk commented 1 month ago

Running forge 7.5.0 with either the ts, mts or cts extension on the config file still causes errors. Using the .mts extension continues to cause the Could not find any make targets configured for the "linux" platform. error. Cts prompts esm error as expected, and with standard ts I get require() of ES modules is not supported..

SpacingBat3 commented 1 month ago

@coredev-uk I think that's the problem of the dependency Forge uses, it might defaults to require. Same might go with others… Maybe also describe what running Forge with DEBUG="*" shows? Maybe Forge will print you the config it interprets and something around it is wrong? Also try out the example folder and check if that works, it's been made to be functional and show how this maker can be integrated with the Forge and application. I don't really want to support CJS anymore, many projects were moving from it entirely already. We also have older builds for now if you need CJS, those aren't extremely outdated and if so, I might still work on backporting them (maybe legacy branch or sth?).

SpacingBat3 commented 1 month ago

That being said, standard forge.config.js or non-class loading (i.e. {name: "@reforged/maker-appimage"} in Forge config) that is actually handled by Forge directly should work just fine, other stuff is dependant on other projects and how Forge's utilizing those as dependencies in reality…

coredev-uk commented 1 month ago

Yeah that worked, its such a strange thing how and where ESM works properly and does not. Previously I was running type: module in the package.json which led to errors in forge but at random instances. I don't fully understand it all, I just wanted to futureproof the code so we dont have to do ESM later on when everyone stops supporting it.

I'll switch back later to the other method to try and get some logs for you, but I'd imagine its on Forge's end.

SpacingBat3 commented 2 weeks ago

Now with Node's latest changes to require() it might actually be funny to see how ESM will do in Forge… As most stuff is still safe to be resolved synchronously, require() might be actually able to handle ESM modules just fine. That's gonna make it a little bit less of the headache for now. It's still dirty fix made on Node's side for stuff that didn't work before with ESM, but at least it is there. And since what I've implemented in Forge actually uses require first and falls back to import, it might actually use require API in Forge for now and print the warning. I possibly should do a PR that does things the opposite way, yet the problem would be with import() loading CJS in way that's incompatible with require(), creating weird module structure containing stuff like .default.default and putting everything to .default even if it shouldn't be there if we want to keep the same structure between ESM and CJS modules.