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(plugin-webpack): Allow each entrypoint to specify nodeIntegration #2867

Merged

Conversation

chetbox
Copy link
Contributor

@chetbox chetbox commented Jun 9, 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:

Summarize your changes:

Note that this replaces #2856 based on this suggestion.

We have multiple BrowserWindows in our project, some which use nodeIntegration: false and some which use nodeIntegration: true.

Without this change we get the error require is not defined at runtime in the BrowserWindows with nodeIntegration: false.

We didn't need to do this with older versions of @electron-forge/plugin-webpack that used Webpack 4 where we could simply not import any Node JS modules and just use browser-targeted modules.

@electron-forge/plugin-webpack only allows us to set one target for all entrypoints. This change allows overriding the nodeIntegration value for a specific entrypoint. e.g.

  plugins: [
    [
      '@electron-forge/plugin-webpack',
      {
        mainConfig: './webpack.main.config.js',
        renderer: {
          config: './webpack.renderer.config.js',
          nodeIntegration: true, // Implies `target: 'electron-renderer'` for all entrypoints
          entryPoints: [
            {
              html: './src/app/app.html',
              js: './src/app/app.tsx',
              name: 'app'
            },
            {
              html: './src/mediaPlayer/index.html',
              js: './src/mediaPlayer/index.tsx',
              name: 'media_player',
              nodeIntegration: false
            }
          ]
        }
      ]
    ]

This uses Webpack's ability to pass an array of webpack configurations rather than a single configuration.

@chetbox chetbox force-pushed the node-integration-for-entrypoints branch from 3d17fd2 to c982d32 Compare June 9, 2022 14:16
@codecov
Copy link

codecov bot commented Jun 9, 2022

Codecov Report

Merging #2867 (cc960e6) into master (0490472) will decrease coverage by 0.12%.
The diff coverage is 50.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2867      +/-   ##
==========================================
- Coverage   71.69%   71.57%   -0.13%     
==========================================
  Files          79       79              
  Lines        2392     2392              
  Branches      450      452       +2     
==========================================
- Hits         1715     1712       -3     
- Misses        454      551      +97     
+ Partials      223      129      -94     
Impacted Files Coverage Δ
packages/plugin/webpack/src/WebpackPlugin.ts 38.00% <0.00%> (-0.58%) ⬇️
packages/plugin/webpack/src/WebpackConfig.ts 98.71% <100.00%> (-0.05%) ⬇️
packages/maker/base/src/Maker.ts 69.69% <0.00%> (ø)
packages/api/core/src/api/make.ts 79.00% <0.00%> (ø)
packages/api/core/src/api/start.ts 64.04% <0.00%> (ø)
packages/api/core/src/util/hook.ts 75.00% <0.00%> (ø)
packages/api/core/src/api/import.ts 61.76% <0.00%> (ø)
packages/api/core/src/api/package.ts 75.55% <0.00%> (ø)
packages/api/core/src/api/publish.ts 68.42% <0.00%> (ø)
packages/api/core/src/util/out-dir.ts 66.66% <0.00%> (ø)
... and 10 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0490472...cc960e6. Read the comment docs.

@erickzhao erickzhao self-requested a review June 9, 2022 15:05
@chetbox chetbox force-pushed the node-integration-for-entrypoints branch from b42d295 to cb57497 Compare June 9, 2022 15:14
@chetbox chetbox force-pushed the node-integration-for-entrypoints branch from cb57497 to 90f9e9d Compare June 10, 2022 08:57
@chetbox
Copy link
Contributor Author

chetbox commented Jun 14, 2022

@malept, is this more in line with what you had in mind based on your comment here?

@chetbox chetbox force-pushed the node-integration-for-entrypoints branch from 90f9e9d to 2a1219e Compare June 21, 2022 14:45
@chetbox
Copy link
Contributor Author

chetbox commented Jun 21, 2022

I've rebased these changed and fixed conflicts with the latest changes in master.

I'm currently maintaining a fork of this here in case others need it: https://www.npmjs.com/package/@electron-forge/plugin-webpack

Any feedback on if this is the right approach would be appreciated.

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.

Hey @chetbox, thanks for submitting this PR! Overall, I think your implementation looks good - changing the rendererConfig return from a single config to an array of configs and handling the nodeIntegration at each entryPoint seems like a fine approach.

Would you mind rebasing this PR against current main, so we can get a fresh run of the test suite? We've merged some webpack work since you opened this PR, and I just want to make sure that the changes still pass with this new config generation. If the tests go green, I'm fine approving and merging 🙇‍♀️

@chetbox chetbox force-pushed the node-integration-for-entrypoints branch from 2a1219e to cc960e6 Compare July 18, 2022 22:37
@chetbox
Copy link
Contributor Author

chetbox commented Jul 18, 2022

Thank you kindly for the review @VerteDinde! I've rebased the changes against master. There seem to be some CI errors with the fast-tests but I don't believe those are to do with the changes here. If there's anything I've missed please let me know.

@chetbox
Copy link
Contributor Author

chetbox commented Jul 26, 2022

Hey @VerteDinde! Is this good to go or do you need anything else from me?

@VerteDinde
Copy link
Member

@chetbox Ah, I missed that the test re-run had passed! I'll approve and merge - thanks again for submitting this PR! 🙇‍♀️

@johannesgiani
Copy link

❤️ 🙏 I've been waiting for this!

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