-
-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
add @electron/rebuild to the dependencies #3736
Comments
if we do this (I'm still not convinced ...) we should move it beside "optionalDependencies": {
"electron": "^35.0.0"
}, |
yes, you are correct. that is really the right place |
most of our module developers are not developers, and our users know even less. whats the harm in trying to add a little stability while we keep current |
Which modules have the mentioned problem?
Modules like MMM-KeyBindings already solved it 2018 (maybe even before) this way:
This works with old and new versions of electron-rebuild. I'm using this module with an evdev device without any issues. Reading the electron version and than use it as parameter for electron-rebuild should work for other modules too, right? So maybe we only should tell the developers how to do it? |
keybinding has my postinstall, but it's mmm-navigate just had the problem |
find the modules w node_helpers, and use onoff,gpio pir, touch, navigate, |
i would never have figured out your env |
Does it? It looks very different to yours.
What's the right one? MMM-KeyBindings already uses @electron/rebuild.
If someone (like me) uses only one module with electron-rebuild there is no problem?
Yeah, but on my setup I don't use the gpio. So it's difficult to test it by myself.
The harm could be that if we introduce it, module developers will build on it. If it turns out later that it wasn't the best solution, we'll have problems taking it back. I am not against improving the situation. But before discussing a concrete solution, I would just like to understand better in which scenarios and why the problems occur. Perhaps there are other solutions. But it shouldn't be up to me, I'm a non-expert when it comes to electron-rebuild. So your decision shouldn't depend on me. I'm just asking questions to understand it better 😀 |
What problem do you want to solve with this change?
modules which require hardware access (pir, sensors, ...)
use low level libraries that require binary compatibility with the node engine running. either
nodejs in serveronly mode
or electron in full mode
during module install the author must choose which mode the module will be used in. most 'assume' full electron mode
the older versions of er(electron rebuild) worked when loaded into the module node_modules tree, it somehow found the electron version. you could also specify a parameter.
the newer er versions do not successfully find the electron version unless it is located in the same node_modules folder
so module install fails, and things change every quarter on our release, and the default upgrade doesnt take this impact into account
my upgrade script does an npm install for all modules with dependencies. i wrote a post-install script used by most of the modules, which checks for er in the MagicMirror node_modules folder, and if not present installs it (now with the wrong npm name )
its not our users fault we keep whacking the system every quarter. and its not the module authors fault either
interestingly, in the serveronly mode, i haven't seen reported issues with chromium/firefox ering used as the browser, even when er done on latest electron level
What do you think is the correct solution?
ship the matching electron-rebuild library with magicmirror
we use it in test, so only have to move from devdependencies list to dependencies list
Participation
Additional comments
No response
The text was updated successfully, but these errors were encountered: