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

compatibility with electron 10 #145

Open
Tobias-Keller opened this issue Sep 5, 2020 · 7 comments
Open

compatibility with electron 10 #145

Tobias-Keller opened this issue Sep 5, 2020 · 7 comments

Comments

@Tobias-Keller
Copy link

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
Copy link
Member

jviotti commented Sep 9, 2020

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
Copy link
Author

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
Copy link
Member

jviotti commented Oct 5, 2020

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
Copy link
Member

jviotti commented Oct 5, 2020

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
Copy link
Member

jviotti commented Oct 5, 2020

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?

@Tobias-Keller
Copy link
Author

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 added a commit that referenced this issue Oct 5, 2020
@jviotti
Copy link
Member

jviotti commented Oct 5, 2020

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

jviotti added a commit that referenced this issue Oct 5, 2020
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

2 participants