Commonjs support

[duzda]

Hi,

this PR adds commonjs support. There are a few changes to allow supporting both esm and cjs together with types, so hopefully both now works and gets automatically determined by the parent package.

Making the example work was quite a challenge, but should be hopefully in the same state as before.

[SpacingBat3]

Honestly, I'm looking forward to drop CJS entirely in packages (because of this I mostly bumped the version) I maintain and encourage people to go with one package system that is supposed to be the standard one: ESM. Especially given Electron's already claims an OK support for it.

Honestly, I would love to hear your opinion on this topic. My stance might be a bit opinionated as I kinda perceive non-ES stuff in Node complicating the lang even more. I'm also unsure if CJS will be actually used in modern JS ecosystems anymore, given it's non-standard I kinda perceive supporting it the same case like browser script still going for that IE support 😛… I guess maybe ESM is still kinda immature when it comes to Node and Electron stuff, but still there are popular packages that declared to drop it even before TypeScript has implemented Node16 so interop of ESM was possible with Electron. As this case is already resolved, I'd say dropping CJS might be kinda OK at the current state.

[SpacingBat3]

Another thing I don't like with this PR is that it enforces NPM as a package manager to at least exist for some scripts to run. I'd rather avoid calling any package manager directly, I go with that policy to keep less things associated to specific package manager and allow people to easier switch between package managers. It's more common for this reason for my scripts to have almost duplicate content than to npm run some scripts within other scripts.

[duzda]

I see, I thought I needed to use CJS to support some legacy packages, but it seems like even in my own use-case migrating to ESM has been way easier. It still might be usable to someone in the future, therefore it's up to you whether to support CJS or push everyone to ESM.

I agree with the NPM issue, I might look into it, if there's an interest to support both. I have been also thinking about using rollup, but decided against using any dependencies.

[duzda]

Oh yeah, now I remember why I did this in the first place, all the official makers use CJS, therefore it just made sense to implement CJS as well. CJS is also used when generating default electron-forge template.

[SpacingBat3]

(…) all the official makers use CJS, (…)

I'm aware of that, in fact Forge normally loads module stuff as CJS (with require calls), which is why you need the ESM config as Forge actually implements a concept of loading a config in many other formats via the loaders – for most loaders you might need a dependency, but in case of ESM scripts it is unnecesary. Honestly I think it would be benefitial for Forge to move to ESM to support CJS/ESM interop better with the external modules.

That being said, ReForged conceptually exists to break some coding rules Forge uses right now, so we aren't limited to their way of doing things, but actually are implementing stuff with our own ways. ESM is kinda beneficial IMO as well, since CJS relies on synchronous loading, while ESM can do other stuff than waiting for the IO due to its async nature. It's another thing that might help constructing the scripts that are better optimized for doing its job.

You might also see that I also implement a method of passing additional args to maker that kinda also allow to leak some params or for additional callbacks for the Forge API interfaces that would like to make use of it… When you think of that, it kinda reminds me of one of M$ business strategies that were supposed to eliminate the concurency or FOSS, without the part that I aim for doing so (I'm more willing to standarize it in some way, in fact.). This is yet another thing that shows I don't want myself to limit to official Forge toolkit.

One thing I'm aware of is that my makers are mostly limiting people of constructing their config in one specific way, as both placing it in JSON and CJS/JS scripts may not work properly right now.

[duzda]

I totally agree with all the points you've listed, forge should move to ESM as the default option, however that's not the case as of right now. The only reason I am suggesting such change is to make the AppImage packager more accessible, since there's no real alternative as of right now. I'm definitely not happy about the official packaging options, however I don't think supporting ESM only is the proper way, as supporting both is really small amount of work, but the gain can be significant, ESM users aren't loosing anything, since the async import is still there and should work.

[SpacingBat3]

however I don't think supporting ESM only is the proper way, as supporting both is really small amount of work, but the gain can be significant

I don't see much of the gain I think, I guess it might be around of formats you may use for the config (I guess ESM might restrict that one thing quite drastically for the time being). But I guess this is what I think I decided for now:

• Right now, I won't merge this. I don't want to have ReForged with double of the code due to 2 packaging APIs. Imagine that every developer would provide both dlls and so libs, as well as both exe and elf format of the binaries, this would be hilarious even if it would improve the portability IMO (the double standard in JS is going so wrong nowadays, especially when you compare some of the newer ESM functionality to Node API you may find it is no longer necessary to use it - and this goes sometimes the same case with all those shiny libs that extended the language).

• Because of the reasons above, I might work on 3.x.y of AppImage maker. I dunno for now whenever I will backport all of the features of 4.x.y to it, but I should at least keep it up-to-date with the latest Electron Forge or other dependencies. At least if there's any interest in keeping CJS maintained.

• I think I might also contribute to Forge directly. It sucks ESM support in Forge tools is quite limited to the specific config syntax, especially when I think fixing this wouldn't be that much of the hassle on their side, at most it could only require porting some funcs to ESM. And stuff like makers is already async in some places, so it is even possible to write a wrapper class that would contain the required properties re-defined, but in case of make method, it would wrap around the ESM version. Moreover, having this fixed on their side and more stuff made to work in async-like manner, we could go as crazy as resolve configs that are actually promises when imported even with CJS (that could be useful to people depending on ESM or async stuff when resolving it, so for sure that would be much of the use; this is again possible right now in ESM thanks to its async structure around modules and existence of top-level await).

Anyway, this is my opinion and idea how to handle the CJS support or/and drop it when it won't be necessary from the perspective of the users to care about the module loader, as Forge will be capable to load ESM and CJS the same way.

[SpacingBat3]

I think I might also contribute to Forge directly.

FYI, see SpacingBat3/ElectronForge. With the set of changes in feat/esm-modules, there are no benefits of supporting both CJS / ESM in case of Forge modules.

Also, I guess this is safe to be closed, at least for now. Feel free to share your thoughts, I'm willing to hear out what's your stance on that topic.

[duzda]

Sorry for late reply,

Right now, I won't merge this. I don't want to have ReForged with double of the code due to 2 packaging APIs. Imagine that every developer would provide both dlls and so libs, as well as both exe and elf format of the binaries, this would be hilarious even if it would improve the portability IMO (the double standard in JS is going so wrong nowadays, especially when you compare some of the newer ESM functionality to Node API you may find it is no longer necessary to use it - and this goes sometimes the same case with all those shiny libs that extended the language).

This is totally understandable and up to you really. It's just something I wanted for my project and figured it might be beneficial upstream as well. I can see it kinda "bloats" the library and therefore may not be suitable. By looking at what most other projects do, they usually just provide CJS, which goes pretty much against everything you've mentioned.

Because of the reasons above, I might work on 3.x.y of AppImage maker. I dunno for now whenever I will backport all of the features of 4.x.y to it, but I should at least keep it up-to-date with the latest Electron Forge or other dependencies. At least if there's any interest in keeping CJS maintained.

Honestly, I think such solution sounds really awful as it doubles the work, my idea was that the amount of works stays pretty much the same but both types get supported.

I think I might also contribute to Forge directly. It sucks ESM support in Forge tools is quite limited to the specific config syntax, especially when I think fixing this wouldn't be that much of the hassle on their side, at most it could only require porting some funcs to ESM. And stuff like makers is already async in some places, so it is even possible to write a wrapper class that would contain the required properties re-defined, but in case of make method, it would wrap around the ESM version. Moreover, having this fixed on their side and more stuff made to work in async-like manner, we could go as crazy as resolve configs that are actually promises when imported even with CJS (that could be useful to people depending on ESM or async stuff when resolving it, so for sure that would be much of the use; this is again possible right now in ESM thanks to its async structure around modules and existence of top-level await).

Anyway, this is my opinion and idea how to handle the CJS support or/and drop it when it won't be necessary from the perspective of the users to care about the module loader, as Forge will be capable to load ESM and CJS the same way.

FYI, see SpacingBat3/ElectronForge. With the set of changes in feat/esm-modules, there are no benefits of supporting both CJS / ESM in case of Forge modules.

Sounds interesting, I will definitely checkout the changes you've made to electron-forge in more detail once I'm more free.

[SpacingBat3]

Sounds interesting, I will definitely checkout the changes you've made to electron-forge in more detail once I'm more free.

Also, for the current status of this at the upstream, see electron/forge#3582.

[SpacingBat3]

I think electron/forge@cc0d7d6 will be closing this for good. My hopes are at this state, just dropping ESM for CJS won't affect most use cases at all, and only when importing the module on your own.

[duzda]

Thank you for the follow-up, this is really some good news! I hope the whole stack moves into this direction rather quickly.