feat(core): support ESM Forge module loading

[SpacingBat3]

• I have read the contribution documentation for this project.
• I agree to follow the code of conduct that this project follows, as appropriate.
• The changes are appropriately documented (if applicable).
• The changes have sufficient test coverage (if applicable).
• The testsuite passes successfully on my local machine (if applicable).

Summary

This PR focuses on bringing the initial support loading Forge modules (plugins, makers, publishers etc.) that use ESM for their module format. It should also contain some cosmetic changes like rename of require-search to import-search (to reflect it isn't around the require API only anymore), improvements around the helper so it won't resolve path for modules passed by their names and introduces new function, dynamicImportMaybe, that will also try doing require first and falling back to ESM when it fails (I chose to require first so I won't have to resolve default in CJS import).

The main motivation on that is to allow third-parties as well as official Forge modules to be able to effortlessly utilise the ESM syntax entirely. While this is possible for now with the Forge as well, it requires of already importing the class to the config on your own.

There are currently a few things that could be considered to be worked on in the future, but are not really necessary from what I've been testing:

• Resolving directory paths for import in order to load them properly. This could be useful when makers are installed in unusual paths that even make Node not to be able to find modules when they are placed that way. Plus, there could be some issue with actually finding what to import given how complex it became in Node to resolve imports, plus resolving internal imports (starting in #) would probably also have to be supported as well. It might not be worth of the efforts for now.

• (Maybe) dropping dynamicImport or making it private, since it doesn't seem to be used outside of its own module scope anymore.

• Improving some tests, so they have simpler logic. Currently, I focused more on preserving on the logic that tests actually uses and only changing them by making them async or use some wrapped stuff. This should make them work essentially similar way while fixing false negatives introduced by some drastic changes, like making some synchronous functions async or renaming a module. Done in 9229bff.

I should also mention, the changes in this repo were tested by me (locally) with my own implementation of ESM maker.

Changes around the tests

Of course, I had to modify tests for require-search and publish-spec, due to changes in module names and switching some previously synchronous code to be async. For now, testing only the workspace @electron-forge/core, I've seen similar number of tests failing on both official implementation and this fork, and I think they were caused because Forge expected to run tests from a root project than a workspace, as I saw those were mostly caused due to missing scripts in workspace package (so it's nothing serious). I guess it's out of the scope to fix that tho.

Additional notes

I hope my commit messages as well as their classification and even this PR name fits well the policy of this project. This is my first time to contribute to Forge, so I'm definitely learning how to do stuff right while trying to avoid mistakes as much as I could 😄️.

[erickzhao]

Overall, this PR seems pretty reasonable to me and the diff is pretty small minus the function renames.

[MarshallOfSound]

Leaving some comments but because this has been open for a while I'll push up the changes 👍

[MarshallOfSound]

This is dodge and can go if we follow the above comment

[SpacingBat3]

@MarshallOfSound Maybe it would be better to actually replace the original promises at this part in the array (and go with traditional for loop), but still keep it all as an array? This way, if plugin.init throws an error, it is further handled (I guess that's the only part I skipped handling).
I really liked the elegance of for await in my solution. And I don't see much reason at await-ing promises during the class creation, I think it's best to leave that when we actually need to depend on the promise result. IMO this over-complicates the code without much benefits (other than ending with the rejected promise immediately).

[erickzhao]

Looks like template tests are failing with the following because this.baseDir in each bundler plugin is not instantiated on time. I'm not sure what the root cause is exactly, but d65e18c seems to be the trigger.

I can't reproduce locally either.

TypeError [ERR_INVALID_ARG_TYPE]: The "path" argument must be of type string or an instance of Buffer or URL. Received undefined
    at Object.rm (node:fs:1248:10)
    at Object.remove (node_modules/fs-extra/lib/remove/index.js:9:24)
    at /Users/distiller/project/node_modules/universalify/index.js:8:12
    at new Promise (<anonymous>)
    at Object.remove (node_modules/universalify/index.js:7:14)
    at /Users/distiller/project/packages/plugin/webpack/src/WebpackPlugin.ts:168:20
    at _TaskWrapper.namedHookWithTaskInner (packages/plugin/base/src/Plugin.ts:59:13)
    at /Users/distiller/project/packages/api/core/src/util/plugin-interface.ts:117:48
    at _Task.taskFn (packages/utils/tracer/src/index.ts:51:36)
    at _Task.run (node_modules/listr2/dist/index.cjs:2063:35)

[SpacingBat3]

I think I might found the cause in code of the undefined behaviour during the object creation, some code seems to be dependant on promise state yet code doesn't reflect this.

I still personally think the past implementation was slightly better as it would only make code that's actually dependant on the promise result to fail than make whole class to fail and probably mistakes like this would also be avoided, what I would change in it is to make plugin.init() also a part of the promise that has to pass to guarantee the further code will end up with the correct plugin objects that are fully created and initialized. Eventually I would still make plugins a Promise type, but rely on their completion within the create function, so JS/TS actually forces await in places there's some dependence on it.

[SpacingBat3]

I might be wrong, since I've only briefly looked at the code, but isn't this code somewhat unsafe at this point, in a way this.plugins state is unknown (it might be still empty/uninitialized) since it doesn't rely on this._pluginPromise completion? You probably need to init plugins as part of the promise to guarantee it will happen.

[MarshallOfSound]

This is safe theoretically because the constructor is private and the only way to make an instance is via the static create method that ensures the property is initialized.

This is a fairly standard pattern

[SpacingBat3]

@MarshallOfSound oh, I apologize for misunderstanding and not being straightforward enough. What I meant is that since the constructor is still used internally to create a base class is that within this constructor there's a for loop that seems to be executed fully synchronously, yet it depends on this.plugins that is actually initialized asynchronously… This means, this loop won't do anything if the this.plugins array is still empty and the async code won't execute before the synchronous (from my understanding of JS this is going to be the case almost everytime, as the next tick is considered at the end of the constructor… but I guess this might as well be undefined behaviour and possibly a mistake, at least if I understand the code correctly).

You probably want to place plugin.init somewhere where the code is still async and this.plugins is guaranteed to be non-empty…

[MarshallOfSound]

Oh I see, sorry reviewing diffs on my phone isn't super easy.

Yeah moving this init loop up into the promise then above should fix that. Missed that when writing this

[rtritto]

Is this PR complete to be merged?

[SpacingBat3]

@rtritto I think so, I guess I'm awaiting more reviews / @MarshallOfSound comment on that (if he didn't noticed the changes within the PR). I personally don't think I want to change anything, unless some other dev has some idea or want to discuss some stuff here I'd say it's ready to be merged.
I might periodically update the branch tho to ensure there's nothing breaking when merging it on top the latest code, or resolve conflicts if I'll encounter any.

[only-issues]

Can this PR still be merged?