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): don't specify resolve.modules by default #2149

Merged
merged 1 commit into from
Aug 6, 2021

Conversation

kamontat
Copy link
Contributor

@kamontat kamontat commented Feb 6, 2021

  • 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:

Specify modules in resolve will cause application not resolve to correct version of dependencies

Fixes #1845.
FIxes #1929.

@codecov
Copy link

codecov bot commented Feb 6, 2021

Codecov Report

Merging #2149 (54b9ef6) into master (fe7d728) will decrease coverage by 2.37%.
The diff coverage is n/a.

❗ Current head 54b9ef6 differs from pull request most recent head 67079dd. Consider uploading reports for the commit 67079dd to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2149      +/-   ##
==========================================
- Coverage   75.08%   72.71%   -2.38%     
==========================================
  Files          91       74      -17     
  Lines        2348     2221     -127     
  Branches      440      420      -20     
==========================================
- Hits         1763     1615     -148     
+ Misses        476      446      -30     
- Partials      109      160      +51     
Impacted Files Coverage Δ
packages/plugin/webpack/src/WebpackConfig.ts 97.61% <ø> (-1.12%) ⬇️
packages/plugin/webpack/src/util/once.ts 12.50% <0.00%> (-87.50%) ⬇️
packages/api/core/src/api/start.ts 64.47% <0.00%> (-23.19%) ⬇️
packages/api/core/src/api/publish.ts 69.73% <0.00%> (-22.37%) ⬇️
packages/api/core/src/api/install.ts 78.20% <0.00%> (-16.67%) ⬇️
packages/api/core/src/util/rebuild.ts 92.85% <0.00%> (-7.15%) ⬇️
packages/api/core/src/api/make.ts 81.72% <0.00%> (-6.58%) ⬇️
packages/api/core/src/api/init.ts 91.17% <0.00%> (-6.39%) ⬇️
packages/maker/appx/src/MakerAppX.ts 55.35% <0.00%> (-1.79%) ⬇️
... and 28 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 fe7d728...67079dd. Read the comment docs.

@malept
Copy link
Member

malept commented Feb 6, 2021

Can you please provide a minimal example repository of where this is a problem?

@kamontat kamontat changed the title Remove modules in resolve webpack fix: shouldn'y specify resolve.modules in webpack Feb 6, 2021
@kamontat
Copy link
Contributor Author

kamontat commented Feb 6, 2021

Can you please provide a minimal example repository of where this is a problem?

@malept Do I have to create full project with electron-forge or just create simple project with webpack and how it create the problem?

@malept
Copy link
Member

malept commented Feb 6, 2021

A minimal Electron Forge project would be preferable.

@kamontat kamontat changed the title fix: shouldn'y specify resolve.modules in webpack fix(plugin-webpack): shouldn't specify resolve.modules in webpack Feb 7, 2021
@kamontat
Copy link
Contributor Author

kamontat commented Feb 7, 2021

@kamontat
Copy link
Contributor Author

kamontat commented Feb 9, 2021

@malept When is will be merged? it kind of block my release right now.

@malept malept changed the title fix(plugin-webpack): shouldn't specify resolve.modules in webpack fix(plugin-webpack): don't specify resolve.modules Aug 6, 2021
@malept malept changed the title fix(plugin-webpack): don't specify resolve.modules fix(plugin-webpack): don't specify resolve.modules by default Aug 6, 2021
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.

It's unclear what the side effects of this change will be, but I guess we'll find out.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants