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

[DiceDB Console] Setup NextJS with Electron #69

Merged
merged 13 commits into from
Oct 15, 2024

Conversation

AjayPoshak
Copy link
Contributor

@AjayPoshak AjayPoshak commented Oct 11, 2024

Summary

  • Setup electron's main.js file to load NextJS app running on http://localhost:4000
  • Created two routes "/" & "/dashboard" to verify routing
  • TS, ESlint config setups by extending existing configs

Not done in this PR

  • Tailwind config setup
  • Node server setup
  • Data fetching example

@AjayPoshak AjayPoshak marked this pull request as draft October 11, 2024 15:58
@AjayPoshak AjayPoshak marked this pull request as ready for review October 13, 2024 10:14
Copy link
Contributor

@KaviiSuri KaviiSuri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Love the idea of using nextjs here!
Perfect unexpected use case of it!

@@ -0,0 +1,8 @@
/** @type {import('postcss-load-config').Config} */
const config = {
plugins: {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey, can we import tailwind config same way we did for playground? Would be better to have common tailwind base config

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't really understand tailwind yet, so not sure if this config will be enough to support theming. I have a separate task for this in our milestones

Copy link
Contributor

@KaviiSuri KaviiSuri Oct 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool, I’ll do that since i set it up in playground too, will be faster

"main": "main.js",
"type": "module",
"scripts": {
"dev": "concurrently --kill-others \"next dev -p ${PORT-4000}\" \"electron .\"",
Copy link
Contributor

@KaviiSuri KaviiSuri Oct 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldnt we be running next from inside electron? How will this be packaged when we run the built version?

I think what we might is to use the standalone output of next and spawn it

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It makes sense to have standalone nextjs server running and electron app is pointing to it, for both local and packaged versions. There is no significant advantage from just shipping bundled files, not the server in packaged version.

Just one issue - users can run this directly also outside electron if they know the URL which shouldn't be an issue.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer the server runs on the users system, and not us hosting it. Especially because we’ll actual servers if we wanna support load tests

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you mean that the server for nextjs is gonna run in packaged version on the users system, what invokes it in the package version?

there is nothing calling next start

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar to forking a nodejs server, we can call childProcess.spawn('pnpm', ['start']) or something like that.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure that bundling pnpm in the final export is that good of an idea.

id rather just call node server.js on the bundled server output

Copy link
Contributor Author

@AjayPoshak AjayPoshak Oct 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@KaviiSuri That was top of my head. There are so many ways to start node server, definitely we won't bundle pnpm in final build 😆

apps/console-electron/package.json Show resolved Hide resolved
@AjayPoshak
Copy link
Contributor Author

AjayPoshak commented Oct 13, 2024

Love the idea of using nextjs here! Perfect unexpected use case of it!

@KaviiSuri What is your idea of frontend in electron?

@KaviiSuri
Copy link
Contributor

KaviiSuri commented Oct 13, 2024

Love the idea of using nextjs here! Perfect unexpected use case of it!

@KaviiSuri What is your idea of frontend in electron?

I was thinking of mounting just the react app (so an SPA), but using nextjs allows us to use the server capabilities of nextjs directly without needing to write an express server.

so we just use server functions or nextjs api routes for our server needs

@KaviiSuri
Copy link
Contributor

i think this is what you want to go towards here? https://www.saybackend.com/blog/03-electron-nextjs-ssr

@AjayPoshak
Copy link
Contributor Author

i think this is what you want to go towards here? https://www.saybackend.com/blog/03-electron-nextjs-ssr

I haven't thought about using RSC.

@KaviiSuri
Copy link
Contributor

i think this is what you want to go towards here? https://www.saybackend.com/blog/03-electron-nextjs-ssr

I haven't thought about using RSC.

Its not about just rsc, its about using nextjs’s server capabilities, since they are very easy to use for frontend devs, so we dont need a express server.

Copy link
Contributor

@KaviiSuri KaviiSuri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM but the build story doesnt look clear yet, lets handle it in separate pr

@AjayPoshak
Copy link
Contributor Author

LGTM but the build story doesnt look clear yet, lets handle it in separate pr

Build step will be handled while setting up publish flow as I have mentioned in milestones sheet.

@AjayPoshak AjayPoshak merged commit 250a1d3 into DiceDB:master Oct 15, 2024
1 check passed
@AjayPoshak AjayPoshak deleted the setup-next-electron branch October 15, 2024 05:08
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

Successfully merging this pull request may close these issues.

2 participants