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

[discussion]: desktop/renderer-app #1173

Closed
6 tasks done
Tracked by #983
Cyberhan123 opened this issue Dec 6, 2021 · 21 comments
Closed
6 tasks done
Tracked by #983

[discussion]: desktop/renderer-app #1173

Cyberhan123 opened this issue Dec 6, 2021 · 21 comments

Comments

@Cyberhan123
Copy link
Contributor

Cyberhan123 commented Dec 6, 2021

  • Need vite electron plugin support
  • Need ESLintPlugin
  • Need BundleAnalyzerPlugin support ,use Rollup Plugin Visualizer
  • Need process.env support
  • Need support fs-extra
  • Need support extract-zi
@Cyberhan123 Cyberhan123 changed the title desktop/renderer-app [discussion]: desktop/renderer-app Dec 6, 2021
@BlackHole1
Copy link
Collaborator

BlackHole1 commented Dec 6, 2021

Supporting BundleAnalyzerPlugin is in low priority.

@BlackHole1
Copy link
Collaborator

what is vite electron plugin?

@Cyberhan123
Copy link
Contributor Author

Cyberhan123 commented Dec 6, 2021

what is vite electron plugin?

vite not support electron-render target by default, we should transform electron package.

@BlackHole1
Copy link
Collaborator

okey

@hyrious
Copy link
Member

hyrious commented Dec 6, 2021

vite not support electron-render target by default

That's true, but it's maybe not a big problem -- see my experimental repo.

The thing I didn't test is electron-builder -- I don't know if it requires some special code format like not using es modules. Having tracked this here: electron/electron#21457.

@Cyberhan123
Copy link
Contributor Author

Anyother task I missing?

@BlackHole1
Copy link
Collaborator

Anyother task I missing?

none

@Cyberhan123
Copy link
Contributor Author

Cyberhan123 commented Dec 17, 2021

vite not support electron-render target by default

That's true, but it's maybe not a big problem -- see my experimental repo.

The thing I didn't test is electron-builder -- I don't know if it requires some special code format like not using es modules. Having tracked this here: electron/electron#21457.

I found electron use require to get some file,the problem is more partial,If we use vite ,the building for development env
Spend time may not less than webpack

@hyrious
Copy link
Member

hyrious commented Dec 17, 2021

If we only want fast bundling time, then we may choose esbuild.
Tree-shaking is even not a significant requirement, since in electron we always bundle all resources.

In fact, we are using vite because it brings really good dev experience.

@Cyberhan123
Copy link
Contributor Author

If we only want fast bundling time, then we may choose esbuild. Tree-shaking is even not a significant requirement, since in electron we always bundle all resources.

In fact, we are using vite because it brings really good dev experience.

As we all know, vite is esm bunlder,we have three way to reslove the commonjs model (electron,nodeBuildInModel,events), first is write commonjs in code . It's easy but ugly.
second we can write vite plugin to transform the import to require,the last way is we can transform electron node to esm import.
And the last way is my choose, any comments? @hyrious @BlackHole1

@hyrious
Copy link
Member

hyrious commented Dec 21, 2021

we can transform electron node to esm import

What's that mean? Can you provide some code examples?

@Cyberhan123
Copy link
Contributor Author

@hyrious
https://github.com/Cyberhan123/flat/blob/feature/render-vite/desktop/renderer-app/scripts/vite-plugin-electron.ts#L27
It's not enough,It still need to reslove events which in https://github.com/netless-io/flat/blob/main/desktop/renderer-app/src/api-middleware/smart-player.ts#L9

@hyrious
Copy link
Member

hyrious commented Dec 21, 2021

Wow, that sounds promising. I'm upvoting that.

Currently only the electron module needs such hack. We can replace events with eventemitter3 or emittery or even the native EventTarget, that's not a problem.

@Cyberhan123
Copy link
Contributor Author

Wow, that sounds promising. I'm upvoting that.

Currently only the electron module needs such hack. We can replace events with eventemitter3 or emittery or even the native EventTarget, that's not a problem.

okkk

@Cyberhan123
Copy link
Contributor Author

Cyberhan123 commented Dec 28, 2021

fs-extra is an tricky question,I have no idea to solve this problem. Can you guys give some inspiration?

It at: https://github.com/Cyberhan123/flat/blob/feature/render-vite/desktop/renderer-app/src/utils/courseware-preloader.ts

@BlackHole1 @hyrious

@hyrious
Copy link
Member

hyrious commented Dec 29, 2021

You can treat it as electron, this is intended to write some node.js related logic at renderer side. In other words, we are violating the context isolation rule of electron. Let me tell you why:

  • The main reason of using electron, like other guys, is seemingly an easy way to write cross-platform desktop applications. Desktop applications can access the fs or do many other native things. Then why do we have to setup additional IPC to let another process doing some work? That should be a waste of code complexity.

  • The doc actually says: prevent the website from accessing Electron internals. You can see the purpose: some people are using electron to wrap websites into it. However, our app has different approaches in implementing native and web.

  • Another main reason for using node modules at renderer side, is agora-electron-sdk currently work in this way.

@Cyberhan123
Copy link
Contributor Author

fs-extra not same as electron .It's not build in model and if we exclude it vite will not handle the import.

@hyrious
Copy link
Member

hyrious commented Dec 29, 2021

Then we may ask @BlackHole1 .

@BlackHole1
Copy link
Collaborator

This issue is representative.

As of electron@12, electron no longer recommends the use of nodejs(node / node library) and most electron api's in the renderer process.

So we now have a new task, which is to remove all the node(modules) api and some of the electron api from renderer-app

We can use IPC or contextBridge instead.

cc: @hyrious @Cyberhan123

@Cyberhan123
Copy link
Contributor Author

This issue is representative.

As of electron@12, electron no longer recommends the use of nodejs(node / node library) and most electron api's in the renderer process.

So we now have a new task, which is to remove all the node(modules) api and some of the electron api from renderer-app

We can use IPC or contextBridge instead.

cc: @hyrious @Cyberhan123

I can write an new vite plugin to support fs-extra, and next step we can remove all the node(modules) api and some of the electron api from renderer-app, It may cost less

@Cyberhan123
Copy link
Contributor Author

good news render app has change to vite #1265, but I suggest to keep webpack, we may need reveal all the details

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

3 participants