+
Skip to content

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.

@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
@erickzhao erickzhao requested review from VerteDinde and malept June 10, 2022 19:54
@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
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
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.

4 participants
点击 这是indexloc提供的php浏览器服务,不要输入任何密码和下载