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

fix(core): allow the forge.config property in package.json to point to a non-js file #3070

Merged

Conversation

erikian
Copy link
Member

@erikian erikian commented Nov 13, 2022

  • 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 PR makes it possible to specify a config file of any extension in the config.forge property of package.json. While #2993 allows for config files in extensions other than .js, they're only interpreted correctly if you don't specify the config.forge property in your package.json - if you do, the file is interpreted as plain .js.

This seems unexpected and can get on the way if one tries to migrate to, say, a .ts config file just by changing the extension in the package.json like this...

   "config": {
-    "forge": "./forge.config.js"
+    "forge": "./forge.config.ts"
   },

... because Forge expects it to be .js and throws an error. Currently, the entire config.forge must be removed for Forge to use the correct loader for forge.config.ts:

-  "config": {
-    "forge": "./forge.config.js"
-  },

@erikian erikian force-pushed the fix/allow-non-js-forge-config-in-package-json branch 3 times, most recently from 7692615 to 7937934 Compare November 13, 2022 19:25
@erikian
Copy link
Member Author

erikian commented Nov 13, 2022

I'm having a hard time adding a test for this. If I use a .ts config file, it gets transpiled when the test runs, which prevents the whole "non-js file being interpreted as js" error that occurs when forge is run in a project and which I'm trying to test for.

I've create a test fixture with a .yml config file to get around this, but I'm getting the following error both locally and in CI: Error: Unable to use specified module loaders for ".yml".

image

This looks like a bug from #2993. Should I keep this test and wait for this bug to get fixed, or should I just drop the test for now?

@erickzhao erickzhao requested a review from a team November 14, 2022 17:33
@MarshallOfSound
Copy link
Member

@erikian Add yaml-hook as a devDependency for your yaml test to work

@erikian erikian force-pushed the fix/allow-non-js-forge-config-in-package-json branch from af5ce09 to 774b2a4 Compare November 14, 2022 21:41
Copy link
Member

@VerteDinde VerteDinde left a comment

Choose a reason for hiding this comment

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

Thanks!

@erikian erikian force-pushed the fix/allow-non-js-forge-config-in-package-json branch from b741c3a to 7787c3e Compare November 15, 2022 14:09
@erikian
Copy link
Member Author

erikian commented Nov 15, 2022

@VerteDinde For context, I removed an unnecessary bit from the code and one of the tests failed because of a 503 somewhere. Rebased onto main to rerun the tests and now everything works again.

@erickzhao erickzhao merged commit 2e5a8bc into electron:main Nov 16, 2022
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

4 participants