electron-userland / electron-json-storage

:package: Easily write and read user settings in Electron apps
1.43k stars 79 forks source link

compatibility with electron 10 #145

Open Tobias-Keller opened 4 years ago

Tobias-Keller commented 4 years ago

tested today the packe with electron 10 and only can use it if i activate the "enableRemoteModule" webPreference.

in electron 10 they changed the default value of "enableRemoteModule" to false, because there are better ways.

will this be updated in this package, so electron-json-storage uses better methods?

jviotti commented 4 years ago

Hi @Tobias-Keller ,

Thanks for the heads up. I haven't been involved in the Electron.js community lately, but I'll do some research. In the mean-time, what are those better methods that you are mentioning?

Do you have a rough idea of how things should work on Electron.js 10, and how to add this in a backwards compatible way to this module?

Tobias-Keller commented 4 years ago

instead of using the remote module, we can use the ipc ipc compatibility in the main process: https://www.electronjs.org/docs/api/ipc-main/history ipc renderer compatibility: https://www.electronjs.org/docs/api/ipc-renderer/history

why? read this: https://medium.com/@nornagon/electrons-remote-module-considered-harmful-70d69500f31

jviotti commented 3 years ago

So the only place that we rely on this is in order to get the userData path:

const electron = require('electron');
const app = electron.app || electron.remote.app;
...
exports.getDefaultDataPath = function() {
  return path.join(app.getPath('userData'), 'storage');
};

Doing this with IPC would overly complicate this module, as you would need to setup the module in the main process before using it the renderer process instead of just require()ing as it is now. Let me see if I can come up with a another solution...

jviotti commented 3 years ago

OK, so I don't think its going to be easy to completely get rid of remote while letting the module be easily setup. At least for the time being, I'll update the docs to clarify that if you want to run this module on a renderer process without using remote, then you can manually call .setDataPath() on the renderer process (assuming you obtained the data path through a custom method) before calling any other functions. Doing so will make the module not go through remote.

jviotti commented 3 years ago

Actually, are you sure this deprecation has been decided? It seems like it is still under discussion based on https://github.com/electron/electron/issues/21408, and it seems to work fine for me.

What exact Electron version are you testing this on? Is it actually failing for you? Can you provide a test app?

Tobias-Keller commented 3 years ago

Actually, are you sure this deprecation has been decided? It seems like it is still under discussion based on electron/electron#21408, and it seems to work fine for me.

What exact Electron version are you testing this on? Is it actually failing for you? Can you provide a test app?

its in the breaking changes on version 10: https://www.electronjs.org/docs/breaking-changes#default-changed-enableremotemodule-defaults-to-false

so i think you can reproduce this with every electron 10 app without setting enableremotemodule to true manually

jviotti commented 3 years ago

Right, my bad. I was still accidentally running on v9. I'll update the README