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(plugin-webpack): prevent the renderer config from overriding its preload config's target #1853

Merged
merged 7 commits into from
Aug 5, 2020

Conversation

vazra
Copy link
Contributor

@vazra vazra commented Jul 20, 2020

override preload script webpack target to electron-preload

  • 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:
while merging to create a Webpack config for preload script, made the target from the renderer config ignored and manually set to electron-preload

Fixes #1852

while merging to create a Webpack config for preload script, made the target from the renderer config ignored and manually set to `electron-preload`
@vazra
Copy link
Contributor Author

vazra commented Jul 20, 2020

any better ways to fix this? @malept @MarshallOfSound

Copy link
Member

@malept malept left a comment

Choose a reason for hiding this comment

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

This seems OK, although I don't know if there are legitimate use cases to override the preload's target.

Can you add a test for this in packages/plugin/webpack/test/WebpackConfig_spec.ts?

@vazra
Copy link
Contributor Author

vazra commented Jul 20, 2020

Can you add a test for this in packages/plugin/webpack/test/WebpackConfig_spec.ts?

sure, will do. 👍

Added 3 tests
- Renderer config : generates config with custom target - web
- Preload renderer config : generates a production config 
- Preload renderer config : generates config overriding custom target with 'electron-preload'
@vazra
Copy link
Contributor Author

vazra commented Jul 20, 2020

Can you add a test for this in packages/plugin/webpack/test/WebpackConfig_spec.ts?

@malept, added some tests, please have a look.

Copy link
Member

@malept malept left a comment

Choose a reason for hiding this comment

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

This seems fine, but I have suggestions for making the new tests smaller.

packages/plugin/webpack/test/WebpackConfig_spec.ts Outdated Show resolved Hide resolved
packages/plugin/webpack/test/WebpackConfig_spec.ts Outdated Show resolved Hide resolved
packages/plugin/webpack/test/WebpackConfig_spec.ts Outdated Show resolved Hide resolved
@malept malept changed the title fix: override preload script webpack target to electron-preload fix(plugin-webpack): prevent the renderer config from overriding its preload config's target Aug 5, 2020
@malept malept added bug plugin/webpack Issues or pull requests related to first-party webpack plugins/templates labels Aug 5, 2020
@codecov
Copy link

codecov bot commented Aug 5, 2020

Codecov Report

Merging #1853 into master will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1853   +/-   ##
=======================================
  Coverage   72.71%   72.71%           
=======================================
  Files          74       74           
  Lines        2210     2210           
  Branches      416      416           
=======================================
  Hits         1607     1607           
  Misses        443      443           
  Partials      160      160           
Impacted Files Coverage Δ
packages/plugin/webpack/src/WebpackConfig.ts 95.23% <100.00%> (ø)

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 b0ca86a...6f022a2. Read the comment docs.

@malept malept merged commit 8126a73 into electron:master Aug 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug plugin/webpack Issues or pull requests related to first-party webpack plugins/templates
Projects
None yet
Development

Successfully merging this pull request may close these issues.

setting target 'web' in renderer webpack config also make the preload script compile with 'web' target
2 participants