feat(core): support ESM Forge module loading #3582

SpacingBat3 · 2024-04-29T20:48:15Z

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, due to the changes, 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 😄️.

packages/api/core/test/fast/require-search_spec.ts

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 👍

packages/api/core/helper/dynamic-import.js

packages/api/core/src/api/package.ts

packages/api/core/src/util/plugin-interface.ts

MarshallOfSound · 2024-06-27T20:37:00Z

packages/api/core/src/util/plugin-interface.ts

@@ -61,15 +61,15 @@ export default class PluginInterface implements IForgePluginInterface {
    });

    for (const plugin of this.plugins) {
-      plugin.init(dir, forgeConfig);
+      void plugin.then((plugin) => plugin.init(dir, forgeConfig));


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

@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 · 2024-07-03T20:12:46Z

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 · 2024-07-13T12:21:32Z

packages/api/core/src/util/plugin-interface.ts

@@ -61,15 +73,15 @@ export default class PluginInterface implements IForgePluginInterface {
    });

    for (const plugin of this.plugins) {
-      void plugin.then((plugin) => plugin.init(dir, forgeConfig));
+      plugin.init(dir, forgeConfig);


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.

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

@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…

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

This ensures calling dynamicImport will rather result in rejected promise than in unhandled exception.

Replace some require() calls with dynamicImport() to support loading makers/publishers/plugins etc. that are ESM makers

Avoid resolving path to file url in dynamicImport to support importing modules by identifiers.

Try require first, then import, to avoid .default property for CJS.

Merge error messages for imports to easier debug potential failures.

Rename require-search test to import-search and improve testing the promise rejection. Co-authored-by: Erick Zhao <erick@hotmail.ca>

This should fix regression caused by bd6ea70.

SpacingBat3 requested a review from a team as a code owner April 29, 2024 20:48

SpacingBat3 mentioned this pull request Apr 29, 2024

Commonjs support SpacingBat3/ReForged#14

Closed

erickzhao reviewed May 1, 2024

View reviewed changes

packages/api/core/test/fast/require-search_spec.ts Outdated Show resolved Hide resolved

erickzhao approved these changes May 2, 2024

View reviewed changes

SpacingBat3 force-pushed the feat/esm-modules branch 4 times, most recently from 11467e3 to d78caf7 Compare May 9, 2024 15:49

rtritto mentioned this pull request May 30, 2024

[PnP] Cannot find the package "electron" #3611

Open

3 tasks

SpacingBat3 force-pushed the feat/esm-modules branch from d78caf7 to 2cf6d60 Compare June 23, 2024 07:47

MarshallOfSound reviewed Jun 27, 2024

View reviewed changes

SpacingBat3 commented Jul 13, 2024

View reviewed changes

SpacingBat3 force-pushed the feat/esm-modules branch from d65e18c to bd6ea70 Compare July 14, 2024 19:29

SpacingBat3 and others added 9 commits July 16, 2024 21:03

refactor(core): try...catch dynamicImport

535dd55

This ensures calling dynamicImport will rather result in rejected promise than in unhandled exception.

feat(core): support ESM module packages

6ee4eb6

Replace some require() calls with dynamicImport() to support loading makers/publishers/plugins etc. that are ESM makers

fix(core): limit path resolving in helper

1e1b023

Avoid resolving path to file url in dynamicImport to support importing modules by identifiers.

fix(core): re-order dynamicImportMaybe methods

c9e6667

Try require first, then import, to avoid .default property for CJS.

fix(core) fix tests in core

80f247f

refactor(core): merge import error messages

d134a5d

Merge error messages for imports to easier debug potential failures.

refactor(core): improve tests

9fd208e

Rename require-search test to import-search and improve testing the promise rejection. Co-authored-by: Erick Zhao <erick@hotmail.ca>

generic refactors

95683e1

fix(core): Move plugins init to promise.

dc48e4f

This should fix regression caused by bd6ea70.

SpacingBat3 force-pushed the feat/esm-modules branch from 9915270 to dc48e4f Compare July 16, 2024 21:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(core): support ESM Forge module loading #3582

feat(core): support ESM Forge module loading #3582

SpacingBat3 commented Apr 29, 2024 •

edited

Loading

erickzhao left a comment

MarshallOfSound left a comment

MarshallOfSound Jun 27, 2024

SpacingBat3 Jul 13, 2024

erickzhao commented Jul 3, 2024 •

edited

Loading

SpacingBat3 left a comment

SpacingBat3 Jul 13, 2024

MarshallOfSound Jul 13, 2024

SpacingBat3 Jul 13, 2024

MarshallOfSound Jul 13, 2024

feat(core): support ESM Forge module loading #3582

Are you sure you want to change the base?

feat(core): support ESM Forge module loading #3582

Conversation

SpacingBat3 commented Apr 29, 2024 • edited Loading

Summary

Changes around the tests

Additional notes

erickzhao left a comment

Choose a reason for hiding this comment

MarshallOfSound left a comment

Choose a reason for hiding this comment

MarshallOfSound Jun 27, 2024

Choose a reason for hiding this comment

SpacingBat3 Jul 13, 2024

Choose a reason for hiding this comment

erickzhao commented Jul 3, 2024 • edited Loading

SpacingBat3 left a comment

Choose a reason for hiding this comment

SpacingBat3 Jul 13, 2024

Choose a reason for hiding this comment

MarshallOfSound Jul 13, 2024

Choose a reason for hiding this comment

SpacingBat3 Jul 13, 2024

Choose a reason for hiding this comment

MarshallOfSound Jul 13, 2024

Choose a reason for hiding this comment

SpacingBat3 commented Apr 29, 2024 •

edited

Loading

erickzhao commented Jul 3, 2024 •

edited

Loading