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

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

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

SpacingBat3
Copy link

@SpacingBat3 SpacingBat3 commented Apr 29, 2024

  • 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 😄️.

Copy link
Member

@erickzhao erickzhao left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@SpacingBat3 SpacingBat3 force-pushed the feat/esm-modules branch 3 times, most recently from 9229bff to 11467e3 Compare May 9, 2024 15:34
SpacingBat3 and others added 7 commits May 9, 2024 17:48
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants