Add --target electron-renderer (for generating electron preloads) #1928
Conversation
silvanshade
force-pushed
the
electron-renderer
branch
6 times, most recently
from
6645e72
to
ebec518
Compare
silvanshade
force-pushed
the
electron-renderer
branch
from
ebec518
to
8166ba9
Compare
silvanshade
force-pushed
the
electron-renderer
branch
from
8166ba9
to
f7d0a10
Compare
|
Just want to mention that this is ready for review whenever someone gets a chance to take a look at it (/cc @Pauan @alexcrichton). Please see the message I posted awhile back to the #wg-wasm discord channel (linked in the original post above). To summarize though, a Secondly, the I have two examples where I need this target: For the I'd be happy to explain more about this on discord. |
|
Thanks for the PR here! I'm personally somewhat wary to add another output type as I feel we already have a few too many. Is there a standard/accepted way to take "normal JS" and make it suitable for use in electron, and if so could we perhaps recommend that process instead of adding a new output type? Or otherwise, do you know about the stability of electron? How much does this sort of interface change over time? I'm a bit worried that electron isn't the most stable thing and would, in the limit, require some sort of versioning on our end or something like that which would be a bummer. |
That's a reasonable concern. I think to some extent this might be unavoidable though just given the complexity of building and deploying apps that use all of this technology and given the current state of the overall ecosystem. Maybe there is a need to split some part of the code generation out of bindgen so that it doesn't seem as much of a commitment to add a new backend? Not sure how that would work but it might be something to consider.
Not that I know of, unfortunately. The standard tools for repackaging/rewriting JS in this situation would be webpack or babel. In theory it shouldn't be terribly difficult to write a babel transform that would accomplish the same thing, but that would require another separate (and significant) dependency and complicate the workflow. I don't know of any webpack plugin that does this directly but it may be possible to write one. Although both of those alternatives could potentially work, I feel they would be a bit of a heavyweight solution to this problem and come with a worse user experience. The webpack workflow in particular already seems fragile and I've run into a number of issues getting it to work correctly with current versions of all the other tooling (including electron). Some of that might be due to the relative immaturity of the webassembly and module loading support.
I don't think that the preload interface changes very often at this point. The earliest reference I can find to the preload script functionality is in the changelog for Electron It used to be that preload scripts were not required but what happened is node integration for renderer process scripts was disabled in version Preload scripts were supported before that but now they are required for node integration (unless you override some defaults settings making your app insecure and which we shouldn't recommend). Unfortunately you can't do much interesting with electron apps without some level of node integration so it's hard to avoid dealing with this somehow. As a point of reference, the list of targets webpack supports is:
I don't think it's necessary to support all of those targets here; I'd just point out that electron support is important enough to webpack to get it's own target. (Those targets don't help with generating the preload file like we need here though). One final thing I would say is that I think it would be fine if this functionality were to be exposed but marked as experimental or undocumented (similar to the node module support) and subject to future change or removal. I don't really know how many people would even use this. But having it would certainly make building electron apps in Rust more straightforward. |
|
Is it possible to use the bundler output and then run that through webpack with the electron target? I find it pretty unfortunate how there's so many JS environments each with slightly enough tweaks to warrant new targets. You're right that we don't want to support all the webpack targets that are supported, but I feel like the line needs to be drawn somewhere so we're not just constantly chasing whatever the new JS environment happens to be. |
It is possible to do that but the electron target doesn't generate the preloads so it doesn't accomplish the same thing as this. There are two aspects to this which are useful:
Normally, the user is expected to write the preloads file by hand (since everything is JS). That's not such a huge requirement, but here we have exactly the right information to generate it automatically. I'm not sure we have a way to accomplish the step 1) with the bundler. It might be possible using one of the plugins to manually go through and exclude each dependency, but then that requires an awful lot of manual configuration from the user to get electron to work (i.e., use the bundler target, configure the excludes, write the preloads, on top of creating whatever bindings you need), which I worry would turn many people away. But aside from that, I wasn't able to get the bundler target to work for the projects I linked. I spent about a week working with webpack (already having prior experience with it), trying all the different targets, and I spoke with @Pauan about it some as well. I never got it to work properly, just got various different cryptic errors when loading the app. I also ran into a problem with webpack running extremely slow (~10 minutes) on the output generated from
That's fair. However, I do think as far as these things go, electron is a reasonably safe target to consider given the number of large applications that use it (e.g., Slack, Discord, VS Code, etc.) and given that webpack themselves have committed to supporting it. If someone can show that there's a way to do the same thing as this PR with webpack and a reasonably straight forward workflow, I'd be more likely to advocate for that instead of a new target. But I tried and couldn't figure out how to do that. |
|
Hm well so to be clear I also understand basically nothing about electron, so I have no idea what the design space is here. I'm mostly just sort of praying that we don't have to maintain this in wasm-bindgen because, well, I don't think we can maintain this realistically. I can give this a rubber stamp that it's at least isolated from the rest of the codebase but I don't think we can realistically given any sort of guarantee about correctness, reliability, maintenance over time, stability over time, etc. This would likely be very difficult to test in-tree as well so it's not really something we can add to CI as well. Unfortunately we're basically between a rock and a hard place. I think the best we can do is to try to merge support for this but be very clear in the documentation that it may not work and it may regress. (and that reminds me that updating the documentation would be good too!) |
Fair enough. I was working on an update to the original message which addressed this a bit more, linking to some of the relevant background information on why electron works this way and why we can't easily solve this problem just with the bundler target and webpack (along with an explanation of what the webpack
I think that's okay. Being able to write Electron apps in Rust and have them work without one-off ad-hoc postprocessing scripts is still an important step. It will help convince people that Rust is a viable choice for this kind of work. I'm also willing to help maintain this code since it's necessary in order to use electron-sys, which I'm already maintaining and continuing to develop.
Hmm. How are the other targets currently tested? It should be possible to do some sort of testing with Electron but it depends on what is needed. At a high level, one possibility is that Electron can be installed as a
I can do that. Can you point me to where the documentation should be updated? |
|
One other thing, before finalizing this, there is actually an alternative interface I've considered in the last few days that should accomplish the same thing but might be more desirable from a maintenance and testing point of view. I'll describe that when I update with the other info I mentioned. |
|
@alexcrichton I've updated the original post with more info about all of this, along with an alternative approach that I think might be better, especially given your concerns about adding another target. Let me know what you think. |
There was a problem hiding this comment.
Ok, we can try adding this and see how it goes. I think the way this works is:
- If necessary for other features, this will be deleted. You'll get pinged of course though and we can work out what's difficult/etc. I want to point out though that I don't want to hinder other development for this one feature which few of us understand.
- If there's questions/review/etc you'll get pinged (e.g. issues, PRs, ...)
Does that sound ok?
Can this also update the documentation in guide/* with various words about this new update type and how it's somewhat experimental?
| } | ||
|
|
||
| fn js_import_header(&self) -> Result<String, Error> { | ||
| let mut imports = String::new(); | ||
| fn js_import_header(&self, imports: &mut String, preloads: &mut String) -> Result<(), Error> { |
There was a problem hiding this comment.
Instead of taking two out-parameters could this return a structure with named fields that document the meaning of each field?
| @@ -194,9 +198,10 @@ impl<'a> Context<'a> { | |||
| &mut self, | |||
| module_name: &str, | |||
| needs_manual_start: bool, | |||
| ) -> Result<(String, String), Error> { | |||
| ) -> Result<(String, String, Option<String>), Error> { | |||
There was a problem hiding this comment.
This return type is getting unfortunately pretty unwieldy, could this be updated to a named structure with documented fields?
| @@ -182,6 +184,13 @@ impl Bindgen { | |||
| Ok(self) | |||
| } | |||
|
|
|||
| pub fn electron_renderer(&mut self, web: bool) -> Result<&mut Bindgen, Error> { | |||
| if web { | |||
There was a problem hiding this comment.
The variable "web" here probably wants a different name
| @@ -35,6 +35,7 @@ Options: | |||
| --remove-producers-section Remove the telemetry `producers` section | |||
| --encode-into MODE Whether or not to use TextEncoder#encodeInto, | |||
| valid values are [test, always, never] | |||
| --electron-renderer Deprecated, use `--target electron-renderer` | |||
There was a problem hiding this comment.
It's ok to not add this since there's no need to add an insta-deprecated option
|
Thanks for the review. I can make the changes suggested. Before proceeding to finalize and merge this though I would like to implement the alternative proposal I made (see the "Alternatives" section in the original post) for comparison. I'll make a separate PR for that.
Yeah, those conditions sound fine.
Okay, I'll take a look and update the relevant sections. |
What does this PR do?
pkg/output.jspkg/preloads.jswith the omitted imports from 1), but in a format suitable for electron-renderer processes with node integration disabledWhy is this needed?
Background
Electron apps use a multi-process model. For this PR we only need to care about the renderer process. We assume that the primary renderer process app script is
pkg/output.js.The electron renderer process can make use of node modules, but (by default) only if they are provided through a
preloads.jsscript. This is because the primary renderer process script (ourpkg/output.js) does not have access to noderequireand will not resolve node modules. However, thepreloads.jsscript does have noderequirein scope, and can make use of all the normal node functionality.This separation is intended to prevent XSS to RCE attacks. You can read more about it here.
How this PR addresses this issue
The output generated from
wasm-bindgenandwasm-pack, when using node modules, contains imports that userequire. Sincerequireis not available to the primary renderer process script, an electron app using this output will fail to load with an error aboutrequirenot being in scope.The solution to that problem is to omit the imports and to not generate code that uses
require. That's what 1) does.That still leaves the issue of how to make those imports available to the renderer script. That's what 2) does, by generating an appropriate
preloads.jsscript.Can't you just use webpack with one of the
electrontargets for this?No. The webpack
electrontargets do not help with 1) or with 2).What the
electron-rendererandelectron-preloadtargets do can be seen here.For the most part they just disable the generation of chunked modules for certain globals (like
electron, andipc).However, the
electron-renderertarget is still treated as a node target, where the generated modules are included into the final output usingrequire, which is exactly what we don't want.In fact, the webpack
electron-renderertarget does not work with current electron because of this. There is an issue referencing the problem here but no resolution.Demonstration
I've uploaded two branches for illustration purposes.
Branch: webpack-activity-monitor
The first branch is webpack-activity-monitor.
If you checkout the code and go to the
examples/activity-monitor, you can run the following to start the example:This branch adapts the template from rustwasm/rust-webpack-template but only uses webpack for the electron renderer script.
If you start the app, you can see that it doesn't function properly. In fact, it fails exactly at the parts where one of the imports (
os.cpusfromnode-sys) is used. It doesn't throw an error, but the results are null (I'm not entirely sure why it behaves this way instead of throwing an error, but in any case it doesn't work).If you uncomment the following line in the
webpack.config.ts, you might expect that the example would work, since we are using theelectron-renderertarget. Instead, the example still fails, but now it does give an error -- specificallyReferenceError: require is not defined.This demonstrates that the
electron-renderertarget doesn't address the problem this PR is intended to solve.I have tried experimenting with several combinations of the plugins (e.g.,
externals,IgnorePlugin, etc.) to see if it is possible to ignore the imports inpkg/output.jsbut none of them see to do the right thing (see the next section).Branch: webpack-activity-monitor-bundler
The second branch is webpack-activity-monitor-bundler.
This branch also adapts the template from rustwasm/rust-webpack-template and only uses webpack on the renderer script. The difference between this and the other branch is that I preprocess the
pkg/output.jsto strip the imports before being compiled with webpack. (See thepack-app-renderer-postprocessnpm script.)Now, if you run the following, the app compiles and runs as expected:
This demonstrates the following:
pkg/output.jsare the main source of the problemRegarding 2), I've verified that it's possible to use either the
webtarget or thebundlertarget from wasm-pack for electron apps. Using thewebtarget is simpler, but in real world apps it's more likely that thebundlertarget would be used for various reasons. That leads me to propose an alternative interface for this functionality.Alternatives
Rather than adding
--target electron-renderer, we could instead add two separate flags:--omit-module-importsand--generate-preloads. These flags would function independently of the choice of--target.After some consideration, this is probably the better option. Most real world applications will probably end up using
--target bundler, but some may still want to use--target web(especially modern apps making full use of modules or other bundlers like rollup).Furthermore, real world applications are likely going to make use of multiple preload scripts with more complex structure than what we would auto-generate.
In fact, the most conservative solution would be to not even provide a
--generate-preloadsflag but only an--omit-module-importsflag. That might be okay, but it would still be nice to have some sort of feedback as to which modules are omitted so the user an idea as to how they should construct the preloads script. I don't know the ideal way to do this. The omitted imports could be reported on stdout or something at the very least.