build: compile 4 webpackRendererConfigs instead of N #3101
Conversation
Co-authored-by: Samuel Attard <marshallofsound@electronjs.org> Co-authored-by: Keeley Hammond <vertedinde@electronjs.org>
|
@MarshallOfSound lines 250, 277, created a new guard function to assert type let me know if that was not the move |
|
Currently potentially having issue w/ asset relocator patch for the failing tests :( path/to/x is not being shortened to _dirname |
| jsPath: path.join(rendererOut, 'main_window/preload.js'), | ||
| nativeModulesString: '.ab=__dirname+"/native_modules/"', | ||
| nativeModulesString: '.ab=require("path").resolve(require("path").dirname(__filename),"..")+"/native_modules/"', |
There was a problem hiding this comment.
Did you figure out what generates this path? This could probably be resolve(__dirname, '..') instead?
There was a problem hiding this comment.
For this test, we generate the path in injectedProductionDirnameCode() within AssetRelocatorPatch.ts
|
Gentle bump @MarshallOfSound — I tried building forge from this commit and was going to test it against my steps from this comment: #3087 (comment) to validate the change, but was running into an issue with linking, I think. |
| @@ -275,7 +275,7 @@ export default class WebpackConfigGenerator { | |||
| ); | |||
| } | |||
| } | |||
| return webpackMerge(baseConfig, rendererConfig || {}, { entry, output, plugins }); | |||
| return webpackMerge({ entry, output, plugins }, rendererConfig || {}, baseConfig); | |||
There was a problem hiding this comment.
Webpack Merge prioritizes right -> left:
we determine the config target within the baseConfig (i.e 'web', 'electron-preload') and not the one generated from rendererConfig = this.pluginConfig.renderer.config.
There was a problem hiding this comment.
edit: maybe I was wrong and shouldn't have touched this
There was a problem hiding this comment.
@georgexu99 Could you link where you saw the prioritization of merging? I don't see that in the webpack docs - I'd err on the side of leaving is as it was unless we're seeing a specific benefit from it 🙂
There was a problem hiding this comment.
within the first code example, a comment mentions that keys to the right take precedence.
I think these need to be moved because otherwise, the separate targets that this PR evaluates do not get translated into the final webpack config (ex. prior to this change, target: electron-preload target in the baseconfig gets overwritten by target: web in the rendererConfig, even if its a preload-only entrypoint
I'm gonna dig a bit deeper and test whether I can make a functioning webpack app with these changes
georgexu99
force-pushed
the
gxu-build-speed-regression
branch
from
fcef7f9 to
d290067
Compare
Co-authored-by: Samuel Attard marshallofsound@electronjs.org
Co-authored-by: Keeley Hammond vertedinde@electronjs.org
related issue:
#3087
getRendererConfigcurrently returns a separate config per entryPoint: this build speed regression was introduced in 1f45e2c when eachentryPointgained the capability to set its ownnodeIntegrationThis change routes each entry point into one of 4 buckets such that only 4 configs are generated.