Skip to content

[Announcement] V2.0 beta stability poll #733

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

Closed
nklayman opened this issue Apr 28, 2020 · 28 comments
Closed

[Announcement] V2.0 beta stability poll #733

nklayman opened this issue Apr 28, 2020 · 28 comments

Comments

@nklayman
Copy link
Owner

Hello everyone:wave:! I'm getting ready to move the 2.0 beta to RC (release candidate) status as it is long overdue, but I first want to make sure that there aren't any breaking changes requested. If you think that the 2.0 beta is ready for RC then please 👍 this issue. If not, please 👎 and leave a comment explaining why.

Specifically, I'd like feedback on the following major changes:

  • Is the new spectron testing process preferred over the old one? Does anyone have any suggestions for improvement?
  • Does it make sense to disable node integration by default? This means that apps will likely be more secure, but native modules and other electron-provided functions will not work without configuring nodeIntegration.
  • Has it been stable for you? Are there any new bugs that have popped up?
  • Is the migration guide on the release announcement sufficient? Were there any additional changes you had to make? Did it all go smoothly?

Thanks for all your feedback!

@nklayman nklayman pinned this issue Apr 28, 2020
@aleksey-hoffman
Copy link

aleksey-hoffman commented Apr 30, 2020

I think electron-builder should not have node integration disabled by default.
By messing with Electron properties it adds an unnecessary complication layer. All Electron settings should be in 1 place - in the main Electron process - so that when you need to change things like node integration you didn't have to waste time figuring out where's what and why does electron-builder docs conflict with Electron docs. And what if I need to disable it only for 1 renderer but not the other ones? Will it override webPreferences.nodeIntegration of every window? People will flood you and the Electron repo with questions asking why don't their modules work anymore.

Please don't add unnecessary complexity layers.

@rathboma
Copy link
Contributor

To talk to these points:

  • I've never used the testing process yet, so sure that's fine :-)
  • Please do not disable nodeIntegration. I think most electron apps use this out of the gate.
  • Very stable.
  • Migration guide did me just fine.

Another point of feedback that would be useful:

  • v2 bumps the supported Electron version, but it's unclear to me if we can manually substitute newer electron versions and it will work, or whether we're locked into the recommended one.

@aleksey-hoffman
Copy link

aleksey-hoffman commented May 3, 2020

@nklayman I found a problem with reloading windows in production. I think it would be nice if v2.0 didn't have this problem.

To reproduce

  • Build the app and install it
  • Open installed app
  • Change route and reload the window with Ctrl + R
  • Get white screen

Fix

To prevent this problem, you could add the following in the main:

mainWindow.webContents.on('did-fail-load', () => {
  if (process.env.NODE_ENV === 'production') {
    mainWindow.loadURL('app://./index.html')
  }
})

I added it right above this line and it fixed the problem:

if (process.env.WEBPACK_DEV_SERVER_URL) {
...

@nklayman
Copy link
Owner Author

nklayman commented May 4, 2020

@aleksey-hoffman and @rathboma thanks for your suggestions! Regarding nodeIntegration:

  • It is disabled by default for vanilla electron projects, the 1.x version of this plugin overrides this
  • If enabled, nodeIntegration can turn an XSS attack into full backdoor access to a computer. This is a major security risk, and I'd like to prevent it being enabled on apps that don't need it
  • The reason nodeIntegration has to be adjusted from the vue.config.js is because the build process changes depending on whether or not it is enabled

However, you made some valid points regarding complexity and how many apps will want nodeIntegration enabled. I think a good option would be to add a prompt for nodeIntegration when you install the plugin. This will allow developers to easily choose to enable nodeIntegration when creating a new project should they want that. The default will be false because that is the electron default and the more secure option. Does this seem like a reasonable comprimise?

@aleksey-hoffman as for the bug you reported, does this only happen with the 2.0 beta and not the 1.x version?

@aleksey-hoffman
Copy link

@nklayman regarding nodeIntegration. If 1.x enables it by overriding default Electron config, couldn't you just remove that override completely in 2.x and let developers set it to true in the main process themselves, if they need to?
I don't quite understand why do you want to make it configurable from within vue.config rather than just removing the override you added in 1.x completely. You see what I mean?

As for the bug, it emerges from Vue router and present in any "single page application (SPA)". Electron only sees 1 real page - index.html, so when you change the route to let's say /settings or whatever, and reload the window, Electron will try to load that /settings page but there's no such page, there's no settings.html, it's just a route. So the fix I mentioned above will simply load the index page if the window fails to load. I just tested the fix and it fixed the white screen issue.

Also, thanks for maintaining and developing this repo, it simplified the development and packaging process for me, I'm building a modern file manager app based on it, which I'm gonna open source on GitHub in about a month.

@nklayman
Copy link
Owner Author

nklayman commented May 4, 2020

@aleksey-hoffman The reason I can't just leave it to be configured in the background.js file is because if the webpack build process allows native modules then it won't work without nodeIntegration (even if native modules are not used), so the webpack script needs to know if nodeIntegration is enabled or not. Rather than run a regex on the background.js which is unreliable I just have it be set from the pluginOptions config.

If you use hash instead of history mode does the bug still occur? You can also use history mode for SPA and hash for electron automatically.

I'm glad this plugin was helpful, and what you're building sounds really cool. I've been thinking about adding a showcase of projects built with this plugin and yours would be a great fit once you open source it, give me a ping when you do if you are interested.

@aleksey-hoffman
Copy link

aleksey-hoffman commented May 4, 2020

@nklayman hmm, yeah, I had mode: 'history'. I commented out the previous fix and changed the mode to the line you referenced, and it also fixes that "reloading white screen" issue:

mode: process.env.IS_ELECTRON ? 'hash' : 'history',

Though I'm not sure if changin mode is gonna break anything. Doesn't look like it did from the first glance, but I'm not sure yet.

Yeah, sure thing, I will let you know when I publish it. I've been working on that file manager app for like the past 2 years, it will have lots of amazing features like:

  • Tabs
  • Modern design
  • Auto backups
  • Wireless file streaming to local devices: in 1 click you can make any file available to all devices on the local wifi network for downloading / streaming.
  • Smart global file search, which is not only insanly fast but also handles complex typos, e.g. to find a file called Test_file_1.txt you can simply type something like tesg 1 (with typos, wrong case, missing words, missing symbols, and missing extension) and it will find it almost instantly.
  • Proper drag and drop (you will be able to download any type of file, including videos by dragging it or its URL onto the app)
  • and more...

Wait till you see, it's gonna be really good, it's about 90% done already 🙂

@aleksey-hoffman
Copy link

aleksey-hoffman commented May 4, 2020

@nklayman I finally figured out how to solve the problem where you would get Not allowed to load local resource when you tried to display an image in a renderer window with webSecurity: true.

I posted a solution here

Since you are trying to make electron-builder more secure by default, and since most devs simply set webSecurity: false just to fix the problem with image loading, perhaps we should somehow incorporate this into electron-builder v2.0? Create a custom protocol for loading media and describe how it works in the docs? I've seen many questions on Github and Stackoverflow, asking how to load images without disabling webSecurity but never found a single proper solution until now.

Perhaps, add something like this in the main:

app.on('ready', async () => {
  const mediaLoadProtocolName = `${appName}-load-media`
  protocol.registerFileProtocol(mediaLoadProtocolName, (request, callback) => {
    const url = request.url.replace(`${mediaLoadProtocolName}://`, '')
    // Decode URL to prevent errors when loading filenames with UTF-8 chars or chars like "#"
    const decodedUrl = decodeURIComponent(url)
    try {
      return callback(decodedUrl)
    }
    catch (error) {
      console.error(error)
      return callback(404)
    }
  })
  ...

And then add some instructions to the docs on how to use it in a renderer:

Renderer | Home.vue

<img 
  :src="getSrc('C:/test.jpg')"
  data-path="C:/test.jpg"
>
methods: {
  getSrc (path) {
    return `app-name-load-media://${path}`
  }
}

@skourismanolis
Copy link

This new way of configuring nodeIntegration is confusing. I'm working on an app that can open multiple renderer windows, some of which are from webpages on the internet (i.e. an auth provider). Can I have nodeIntegration disabled in those windows but enabled on others? Does the vue.config.js option even override the params I pass to those windows?

What I have understood by now is that the config option doesn't affect my app, just the build process, so if I have it set to false, I could not use nodeIntegration with any of my windows because webpack will fail, but it it's set to true, I can use it, but it's doesn't override the nodeIntegration parameter I pass to new BrowserWindow. Is that correct?

@nklayman
Copy link
Owner Author

nklayman commented May 7, 2020

@skourismanolis The nodeIntegration option in vue.config.js not only changes the build process but also sets process.env.ELECTRON_NODE_INTEGRATION, which is used to enable/disable nodeIntegration in the electron launch config. This prevents you from having to update the configuration in two places.

@nklayman
Copy link
Owner Author

nklayman commented May 7, 2020

@aleksey-hoffman I've created #742 to collect security-related recommendations for a new section in the docs. Can you repost your suggestion there for better tracking? If possible, I'd appreciate a short description of why switching webSecurity off is a bad idea and maybe any other pitfalls you ran into trying to keep webSecurity enabled. Thanks for your help!

@skourismanolis
Copy link

@skourismanolis The nodeIntegration option in vue.config.js not only changes the build process but also sets process.env.ELECTRON_NODE_INTEGRATION, which is used to enable/disable nodeIntegration in the electron launch config. This prevents you from having to update the configuration in two places.

So, as long as I don't use the process.env.ELECTRON_NODE_INTEGRATION env variable when I spawn my windows, I can control the nodeIntegration manually right?

@nklayman
Copy link
Owner Author

nklayman commented May 7, 2020

Yep, but just beware that you might get errors because of build process changes from the nodeIntegration setting in vue.config.js.

@yvesh
Copy link

yvesh commented May 8, 2020

I've been using the latest Beta versions for some time now, even in production. Hadn't had any issues upgrading from 1.4 and using the latest Electron 8. For testing i prefer Cypress and it's integration into Electron is a different topic, so sadly no feedback on Spectron..

As I am using native modules I have to enable the node integration, but I think it's a good thing to be "more secure" by default.

So far no (v2 related) bugs or new issues showed up, so i would consider it stable. As the app is still in beta and pretty new, it only has a limited user audience from around 300 people, but all operating systems (Linux, macOS, Windows) are represented.

@aleksey-hoffman
Copy link

@nklayman Electron v9 is out. Perhaps electron-builder v2 should support it out of the box?

Though, keep in mind, they switched the default value of app.allowRendererProcessReuse to true in Electron v9. So you will get an error if your app uses any non-context-aware or non-NAPI modules, after updating Electron to v9.

Of course, you could add a temporary override to the main:

app.allowRendererProcessReuse = false

But they will deprecate this override in Electron v11, so maybe developers should do this by themselves, if they want to keep using non-context-aware modules in Electron 9+?

@nklayman
Copy link
Owner Author

V2 already uses electron v9 by default, the latest version just hasn't been published yet.

@mmikhan
Copy link

mmikhan commented May 30, 2020

@nklayman Got any estimate release date yet? 😬

@Sparkenstein
Copy link

Need a bit more clarity on migration. my current testing project uses v1. I installed with simple steps given in master/readme.md. firstly, it prompted 3 electron versions, 4.x 5.x and 6.x. I fixed it with yarn upgrade --latest.
Now for migrating to v2, the doc says replace nodeIntegration:true with nodeIntegration: process.env.ELECTRON_NODE_INTEGRATION

It's not clear how this will change plugin v1 to v2? If you can give clear steps how to get started with v2 I think I can test stability out.

also according to release I see that there are only two changes, testing flow and disabled nodeIntegration. Is that the full changelog? Have you given a thought on adding basic examples for preload, contextIsolation etc.

@nklayman
Copy link
Owner Author

@Sparkenstein I updated the migration guide with instructions on updating the plugin version, I forgot to add that. I added docs on preload with full instructions as well.

@nklayman
Copy link
Owner Author

v2.0 has been promoted to RC and is now the default version!

@nklayman nklayman unpinned this issue May 31, 2020
@nklayman
Copy link
Owner Author

@aleksey-hoffman I think loading local resources works just fine using the app:// protocol which is included. It is also automatically used if you just use ./file.png. Can you try this and see if it works in place of the custom protocol you suggested? If so, I can simplify the docs by just suggesting to use the app:// protocol.

@aleksey-hoffman
Copy link

aleksey-hoffman commented Jun 13, 2020

@nklayman It doesn't work for me. When I try to load a media file, e.g.:

<img :src="app://E:/file.png">

I get this error:

GET app://e/file.png net::ERR_UNKNOWN_URL_SCHEME

It's weird that it changes drive letter from E:/ to e/ on Windows. it doesn't do that in my custom protocol. Perhaps that's what causes the error?

I'm using v1.4.6. Did anything change in v2.0.0? I didn't try it yet.

@nklayman
Copy link
Owner Author

@aleksey-hoffman What happens if you use a relative path? For me those worked but absolute paths didn't.

@aleksey-hoffman
Copy link

aleksey-hoffman commented Jun 13, 2020

@nklayman I just put the image file in the app directory and tried loading it with app://test.png and app://./test.png still the same error:

GET app://./test.png/ net::ERR_UNKNOWN_URL_SCHEME

Is that app protocol supposed to work only in production? Because I'm testing it in development.

I don't think I've deleted any default electron-builder functions from the background.js file, it's still registers the protocol scheme and calls the createProtocol('app') function, and loads the index.html on that protocol. I'm probably doing something wrong, but I'm not sure why doesn't it work like my own custom protocol.

To load files in my app, I'm using the implementation I described here. Maybe that custom protocol is doing something different, I'm not sure.

Anyway, I think most Electron apps are using absolute paths to load media because user files are not located in the app directory, so I'm not sure how useful it is to only have the ability to load relative paths.

@nklayman
Copy link
Owner Author

Okay, I've done some more research and I found out that using app:// works just fine for loading out of the public folder, but for loading absolute paths you need the custom local-resource protocol. I've updated the docs to make this more clear. Thanks for your help debugging @aleksey-hoffman. BTW app:// only works in production, in dev public files will served on the dev server and can be accessed with /image.png. process.env.BASE_URL will automatically be set to either app:// or / in prod and dev.

@aleksey-hoffman
Copy link

@nklayman good to know. By the way, are you planning to add support for Vue 3 when it comes out of the Beta?

I tried updating my app to Vue 3 myself by running vue add vue-next but it broke everything and I don't really have time to figure out how to make it work, still trying to finish the file manager app I told you about...

@nklayman
Copy link
Owner Author

Yes, I'd love for this plugin to support vue 3. I'll take a look at it sometime this week probably.

@realrecordzLab
Copy link

realrecordzLab commented Dec 4, 2020 via email

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

No branches or pull requests

8 participants