Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

.ts config does not load with this package being imported into it #15

Closed
iameli-streams opened this issue Sep 17, 2024 · 13 comments
Closed
Labels
⛔️ blocking:invalid This doesn't seem right ⛔️ blocking:wontfix This will not be worked on 📦️ package:maker-appimage Associated to @reforged/maker-appimage

Comments

@iameli-streams
Copy link

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
Copy link
Author

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

@SpacingBat3 SpacingBat3 added ⛔️ blocking:invalid This doesn't seem right ⛔️ blocking:wontfix This will not be worked on 📦️ package:maker-appimage Associated to @reforged/maker-appimage labels Sep 17, 2024
@SpacingBat3
Copy link
Owner

SpacingBat3 commented Sep 17, 2024

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
Copy link
Owner

SpacingBat3 commented Sep 17, 2024

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
Copy link
Author

iameli-streams commented Sep 17, 2024

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
Copy link
Owner

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
Copy link
Owner

As of the said PR, Forge's already merged a good enough support for ESM here:
electron/forge@cc0d7d6

🎉 Hope to see it in action!

@coredev-uk
Copy link

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
Copy link
Owner

SpacingBat3 commented Oct 1, 2024

@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
Copy link
Owner

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
Copy link

coredev-uk commented Oct 10, 2024

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
Copy link
Owner

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.

@SpacingBat3
Copy link
Owner

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)

I still wonder what's DEBUG=electron-forge:* is showing, this could give some clue how's Forge parsing .mts files. Should say .mjs were working fine even before I worked on patch for Forge, and in fact ReForged's example was relying on loading via .mjs (it still was compiled from .mts).


Also, since I didn't mentioned exactly what is responsible for the loading other extensions, this is basically all related information I thought would be worth mentioning:

@SpacingBat3 SpacingBat3 pinned this issue Dec 2, 2024
@SpacingBat3 SpacingBat3 added ℹ️ status:reproducible This issue can be reproduced and should be worked on and removed ℹ️ status:reproducible This issue can be reproduced and should be worked on labels Dec 9, 2024
@SpacingBat3
Copy link
Owner

Actually looking into this a bit deeper, I see this situation might see very little improvement on Forge's side if rechoir and interpret are the only packages used for handling custom extensions. Plus, .mts is actually not defined by interpret, which currently hard-codes the extensions Forge is using for loading data, hence this error (there was no actual config definition and Forge's fallen back into an empty object I suppose).

I'll close this for the time being as wontfix, since .mts and .ts loading is its own thing in Forge that is very far related to this project's codebase and even somewhat not directly handled by Forge itself, and as long as one of those two is going to be true, this is hopeless to be resolved for now on my side.

@SpacingBat3 SpacingBat3 closed this as not planned Won't fix, can't repro, duplicate, stale Dec 9, 2024
@SpacingBat3 SpacingBat3 changed the title ERR_REQUIRE_ESM .ts and .mts direct config loading is not supported Dec 9, 2024
@SpacingBat3 SpacingBat3 changed the title .ts and .mts direct config loading is not supported .ts config does not load with this package being imported into it Dec 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
⛔️ blocking:invalid This doesn't seem right ⛔️ blocking:wontfix This will not be worked on 📦️ package:maker-appimage Associated to @reforged/maker-appimage
Projects
None yet
Development

No branches or pull requests

3 participants