Plugins relying on module hijacking won't load.

[tjwelde]

While investigating, why hyperline doesn't work in hyper2, I found out, that the main process requires the plugin too. Unfortunately it doesn't hijack common modules, as the renderer does, so it fails.

So, with this PR modules loading is also hijacked on the main process, so that plugins can load.

Problems I see:

case 'hyper/Notification':
case 'hyper/notify':
case 'hyper/decorate':

all return just Object here, because they can not just be required, since they work differently on the main process, than on the renderer. I don't really know, how to solve this, or even if solving that would be needed.

Also the app/plugins.js file is required from the renderer process, but returns a cached copy, so the code doesn't get executed again, but it is still smelly. Especially having the module hijacking twice in the code. Any ideas here?

[chabou]

Thank you so much to have analyzed this issue.

I have no problem to have some copy/paste and some duplicate code.

But 2 things should be improved:

• Adding some comments in both places to ask contributors to update both.
• Can you create your feature branch from our canary branch and not master branch?

[chabou]

Some improvements are needed

[tjwelde]

Ok, I will work on it, when I have some free time. I did base this PR against master, since it felt more like a hotfix to me, but I can still base it against canary, if you want to.

[chabou]

Yes please, every PR should target canary wand will be eventually cherry-picked to master if it can't wait for a normal release cicle: canary -> master

[tjwelde]

@chabou please review again

[tjwelde]

Or maybe @Stanzilla ?

[albinekb]

LGTM 👍

[chabou]

Sorry for the delay

[chabou]

I'll test it ASAP

[chabou]

Nice work @tjwelde!
Thank you so much 🙏

[sahilchaddha]

Hi @tjwelde ,
Running these changes gives error "Cannot find module react" after running yarn run dist . The Mac .app returns error.

Commenting the code for patchModuleLoad(); works well. But with these changes it does not.

[tjwelde]

Hmm, yes you're right. React is placed in the root node_modules, which apparently doesn't get added into the build file. Reading into this it turns out electron-builder is only including files from the app directory and before we execute build, we compile lib into app/renderer/bundle.js.

So, we either add react and react-dom also to app/package.json, or we decouple "frontend hooks" from "backend hooks" at the plugin side, e.g. we have two entry points in a plugin, one for the Electron hooks, one for the Renderer hooks.

What do you say @chabou ?

[chabou]

I can't imagine a case where a plugin needs react or react-dom in the main process.
I would suggest returning an Object for these imports too.

Any case, splitting plugins is not an option, imo.

[tjwelde]

I tried that before, but unfortunately it breaks the plugin at the loading stage.
I think it is weird to load everything a plugin needs for the frontend, while only wanting to load the backend code.

[chabou]

Ok, I reproduced, and this is due to styled-jsx:
TypeError: Super expression must either be null or a function, not undefined at exports.default (/Users/chabou/Documents/Projets/hyper/.hyper_plugins/local/hyperline/node_modules/styled-jsx/node_modules/babel-runtime/helpers/inherits.js:21:11)

I think that we don't have to hijack react and react-dom import on the main process side.
Removing these imports and returning Object for hyper/component did the trick for me.

[tjwelde]

I tried your proposed solution, but it doesn't work after running yarn run dist.
I guess it is that way, since while developing, hyperline is located in a subdirectory of hyper and therefore has access to hypers node_modules. It doesn't have that in the build, since hyperline is located in the users home directory in that case.

[chabou]

@tjwelde It definitely works (don't forget to remove react(-dom) requires).

I made #3067. Please discuss there about how to fix this regression.
We need to fix it quickly because we can't release anymore.