Conversation
MarshallOfSound
left a comment
There was a problem hiding this comment.
Can you explain the case where this doesn't work / present a test case. Unclear what bug is being solved here, the plugin definitely works in it's current form so I'm interested in the specific edge case where it doesn't?
this PR resolve #3934 |
|
@MarshallOfSound Did you get a chance to see the #3934 and my PR? |
|
@dmazzella I think it'd be good to add a test case showing what this PR fixes. It'll also help us catch regressions in the future. 🙇 |
done, i have added tests and all passed successfully |
erikian
left a comment
There was a problem hiding this comment.
@dmazzella following the repro steps in #3934 and replacing node_modules/@electron-forge/plugin-vite/dist/VitePlugin.js in the repro project with packages/plugin/vite/dist/VitePlugin.js built on your branch does result in the app.asar.unpacked folder being created with the following structure:
app.asar.unpacked
└── node_modules
└── better-sqlite3
├── bin
│ └── darwin-arm64-140
│ └── better-sqlite3.node
└── build
└── Release
├── better_sqlite3.node
└── test_extension.node
However, I bisected this regression to #3583, specifically this code was handling the unpacking before it was removed: 5485969#diff-d1d8524b16d69b5d383151b395f4ac4151282b39dac16af4bd833cae8b689c8cL123-L126. If I restore that code, I get:
app.asar.unpacked
└── node_modules
└── better-sqlite3
└── build
└── Release
└── better_sqlite3.node
...which I think makes more sense since build/Release/better_sqlite3.node is the only binding used in runtime as far as I can tell. The previous approach is also faster than the one you're proposing (yarn package takes around 4s for me, versus ~18s in your branch).
Do you think it would be possible to bring your approach closer to what we had before #3583 in terms of performance and behavior? I'm not exactly sure why that code was removed (@caoxiemeihao / @erickzhao probably have the context here), but another option would be restoring it, as long as that doesn't cause any unwanted side effects.
…ing in VitePlugin
|
I updated the PR to add |
|
Thanks for kicking this off. Native module handling for the Vite plugin is being reworked for Forge 8 in #4232, which detects native packages automatically, externalizes them from the bundle, and copies them with their transitive dependencies into the packaged app. Closing this one as superseded by that approach; feedback on #4232 is very welcome. Generated by Claude Code |
Include native module packages in asar for AutoUnpackNativesPlugin compatibility