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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: esm forge.config.js support #3358

Merged
merged 4 commits into from Nov 2, 2023
Merged

Conversation

mahnunchik
Copy link
Contributor

  • 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).

Summarize your changes:

This tiny PR adds ESM config support. Test added. Closes #3350

It is not a breaking change. Be ready for Electron 28 with ESM support! 馃挴

@mahnunchik mahnunchik requested a review from a team as a code owner September 18, 2023 23:01
@mahnunchik
Copy link
Contributor Author

Of course, I understand that it is better to (1) update typescript (2) update target to esnext, (3) update module resolution to esnext too (4) replace interpret and rechoir with something working with esm but this small fix allow to use ESM config without such big changes 馃槈

@mahnunchik
Copy link
Contributor Author

I think the failing Windows tests are not related to the changes.

@mahnunchik
Copy link
Contributor Author

Related #3129

Copy link
Member

@MarshallOfSound MarshallOfSound left a comment

Choose a reason for hiding this comment

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

We cant ship code that depends on string templated eval, that's neither safe nor generally good practice

@mahnunchik
Copy link
Contributor Author

@MarshallOfSound maybe you know how to make quite old typescript to not compile dynamic import?

Or if you plan to upgrade typescript and compiled module type it should be pretty easy.

@MarshallOfSound
Copy link
Member

Make a plain JS file that exports a helper that uses the dynamic import method. Not pretty but better than eval imo

@mahnunchik
Copy link
Contributor Author

@MarshallOfSound if dynamic import is in js file it is compiled to require.... I need help to figure it out.

return Promise.resolve().then(() => __importStar(require(path)));

@mahnunchik
Copy link
Contributor Author

@MarshallOfSound I think I was able to get it to work with the current version of ts. Could you please check the current implementation?

@mahnunchik
Copy link
Contributor Author

Windows tests are fixed!

@mahnunchik
Copy link
Contributor Author

@MarshallOfSound @erikian @erickzhao what can I do to ship this feature?

@mahnunchik
Copy link
Contributor Author

@MarshallOfSound @erikian @erickzhao it is me again 馃槈 What should I do to ship this feature?

@himself65
Copy link

hi, what's the update on this progress?

@erikian erikian requested a review from a team October 17, 2023 23:53
@MarshallOfSound MarshallOfSound merged commit 563fc17 into electron:main Nov 2, 2023
7 checks passed
@Jayatubi
Copy link

Jayatubi commented Feb 20, 2024

Looks like it doesn't work with forge.config.ts.

The dynamicImport would fail with ts version config path and esm on:

code:
'ERR_UNKNOWN_FILE_EXTENSION'
message:
'Unknown file extension ".ts" for E:\project\forge.config.ts'

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.

Support ESM forge.config.js
5 participants