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
SpacingBat3
wants to merge
7
commits into
electron:main
Choose a base branch
from
SpacingBat3:feat/esm-modules
base: main
Could not load branches
Branch not found: {{ refName }}
Could not load tags
Nothing to show
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
erickzhao
reviewed
May 1, 2024
erickzhao
approved these changes
May 2, 2024
There was a problem hiding this 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
force-pushed
the
feat/esm-modules
branch
3 times, most recently
from
May 9, 2024 15:34
9229bff
to
11467e3
Compare
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>
SpacingBat3
force-pushed
the
feat/esm-modules
branch
from
May 9, 2024 15:49
11467e3
to
d78caf7
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
toimport-search
(to reflect it isn't around therequire
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 doingrequire
first and falling back to ESM when it fails (I chose torequire
first so I won't have to resolvedefault
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
andpublish-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 😄️.