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): improve native asset relocation without forking Vercel loader #2320

Merged
merged 24 commits into from
Jul 11, 2021

Conversation

timfish
Copy link
Contributor

@timfish timfish commented Jun 10, 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).

This PR adds a plugin to renderer webpack configurations that patches the code injected by @vercel/webpack-asset-relocator-loader so that it works with Electron/forge.

Main and preload configurations already work without patching!

Closes #2328.
Closes #2154.
Closes #1688.
Closes #1451.
Closes #1424.
Closes #1224.

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.

Love that this is net negative lines of code (before tests)!

@malept malept changed the title Feature: Improved native asset relocation without forking Vercel loader feat(plugin-webpack): Improved native asset relocation without forking Vercel loader Jun 10, 2021
@codecov
Copy link

codecov bot commented Jun 10, 2021

Codecov Report

Merging #2320 (d7e7919) into master (43cbb0a) will increase coverage by 2.40%.
The diff coverage is 80.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2320      +/-   ##
==========================================
+ Coverage   72.19%   74.59%   +2.40%     
==========================================
  Files          76       77       +1     
  Lines        2287     2299      +12     
  Branches      437      436       -1     
==========================================
+ Hits         1651     1715      +64     
- Misses        473      476       +3     
+ Partials      163      108      -55     
Impacted Files Coverage Δ
packages/template/webpack/src/WebpackTemplate.ts 100.00% <ø> (ø)
...ges/plugin/webpack/src/util/AssetRelocatorPatch.ts 76.92% <76.92%> (ø)
packages/plugin/webpack/src/WebpackConfig.ts 98.73% <100.00%> (+1.11%) ⬆️
packages/maker/base/src/Maker.ts 75.67% <0.00%> (-2.11%) ⬇️
packages/installer/darwin/src/InstallerDarwin.ts 31.57% <0.00%> (-1.76%) ⬇️
packages/api/core/src/util/upgrade-forge-config.ts 94.73% <0.00%> (-1.70%) ⬇️
packages/api/core/src/api/init.ts 97.05% <0.00%> (+5.88%) ⬆️
packages/api/core/src/api/make.ts 88.29% <0.00%> (+6.57%) ⬆️
packages/api/core/src/util/rebuild.ts 100.00% <0.00%> (+7.14%) ⬆️
... and 3 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 43cbb0a...d7e7919. Read the comment docs.

@timfish
Copy link
Contributor Author

timfish commented Jun 10, 2021

For reference, here is the PR to fix the issue in the html-webpack-plugin.

@ivancuric
Copy link

Hopefully this will fix a number of other issues including jprichardson/node-fs-extra#863

@timfish timfish marked this pull request as ready for review June 16, 2021 13:09
@timfish
Copy link
Contributor Author

timfish commented Jun 16, 2021

@ivancuric you can try this loader to confirm.

@ivancuric
Copy link

@ivancuric you can try this loader to confirm.

It does! I was hoping it would also fix #1276, and it does somewhat. It previously crashed during compilation of preload scripts:

⠼ Compiling Preload Scripts
An unhandled rejection has occurred inside Forge:
[Error: EISDIR: illegal operation on a directory, read] {
  errno: -4068,
  code: 'EISDIR',
  syscall: 'read'
}

Electron Forge was terminated. Location:
{}

Now the app compiles, but the output seems to be missing one level — it's not in the main_window folder like the other native module:
image
The electron app opens, but with the following error in the console:

external "util":1 Uncaught ReferenceError: require is not defined
    at Object.util (index.js:108240)
    at __webpack_require__ (index.js:790)
    at fn (index.js:101)
    at Object../node_modules/sharp/lib/constructor.js (index.js:102641)
    at __webpack_require__ (index.js:790)
    at fn (index.js:101)
    at Object../node_modules/sharp/lib/index.js (index.js:103045)
    at __webpack_require__ (index.js:790)
    at fn (index.js:101)
    at Module.<anonymous> (index.js:6165)

@timfish
Copy link
Contributor Author

timfish commented Jun 16, 2021

Native modules end up in 3 different locations depending on where they are loaded from:
Main - .webpack/main/native_modules/
Preload - .webpack/renderer/{entry-point}/native_modules/
Renderer - .webpack/renderer/native_modules/

require is not defined means that your renderer bundle has ended up with require(..) in it and require is not available. You'll need to enable nodeIntegration: true and contextIsolation: false or avoid using require in the renderer.

If you share a repository I can probably tell you what's going on rather than guess!

@ivancuric
Copy link

ivancuric commented Jun 16, 2021

Oof. I'll see if I can make a minimal repro since it's a pretty big private repo for a commercial project.

Adding nodeIntegration: true fixed it again somewhat:

Something went wrong installing the "sharp" module

node-loader:
Error: Invalid package D:\dev\novocam-recording-software\node_modules\electron\dist\resources\electron.asar

- Remove the "node_modules/sharp" directory then run
  "npm install --ignore-scripts=false --verbose sharp" and look for errors
- Consult the installation documentation at https://sharp.pixelplumbing.com/install
- Search for this error at https://github.com/lovell/sharp/issues

I actually got it working (somewhat) with #1250 (comment) but I honestly have no idea what's going on there 😅 It also doesnt' really work since it breaks on reload, but I'll continue this conversation there.

@ivancuric
Copy link

Here's a repro repo!
https://github.com/ivancuric/native-repro-sharp

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.

Overall it's good, I have mostly style / refactor suggestions.

Ultimately I think the tests could be refactored to be a little more DRY. I can do that work.

.gitignore Outdated Show resolved Hide resolved
packages/plugin/webpack/src/util/AssetRelocatorPatch.ts Outdated Show resolved Hide resolved
packages/plugin/webpack/src/WebpackConfig.ts Show resolved Hide resolved
packages/plugin/webpack/src/WebpackConfig.ts Outdated Show resolved Hide resolved
packages/plugin/webpack/test/AssetRelocatorPatch_spec.ts Outdated Show resolved Hide resolved
packages/plugin/webpack/test/AssetRelocatorPatch_spec.ts Outdated Show resolved Hide resolved
packages/plugin/webpack/test/AssetRelocatorPatch_spec.ts Outdated Show resolved Hide resolved
packages/plugin/webpack/test/WebpackConfig_spec.ts Outdated Show resolved Hide resolved
packages/plugin/webpack/test/AssetRelocatorPatch_spec.ts Outdated Show resolved Hide resolved
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.

Unfortunately CI / mocha is lying to us, there are definitely test failures. I fixed some but there's two failures checking for the asset relocator patch plugin in the preload config.

@timfish
Copy link
Contributor Author

timfish commented Jun 20, 2021

Just fixed those.

The loader requires no modification to work in the preload so the plugin should not be loaded in those cases.

@malept malept mentioned this pull request Jul 11, 2021
4 tasks
@malept malept changed the title feat(plugin-webpack): Improved native asset relocation without forking Vercel loader feat(plugin-webpack): improve native asset relocation without forking Vercel loader Jul 11, 2021
@malept malept merged commit db8a3f3 into electron:master Jul 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment