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: questions around Server implementation and refactoring #845

Closed
freelerobot opened this issue Dec 5, 2023 · 5 comments
Closed
Assignees
Labels
P0: critical Mission critical

Comments

@freelerobot
Copy link
Contributor

freelerobot commented Dec 5, 2023

Related #798

Q: Given this refactoring, can Jan be run in the following modes:

  1. electron-only: I want to only run Electron with ipcMain process, not Fastify, i.e. is what we currently have still an option going forward?
  2. server-only: I want to only run Fastify server process(es), not Electron.
  3. dual: I want both desktop app & local port: routes to single process
    • Can we have a /web/dependecy-injection.js for dual mode, what is routed to and what is not
  • What's the current implementation enabling?
  • In dual mode, what is the lifecycle of a user request (e.g. createAssistant)?
User -- useCreateAssistant--> ElectronHandler --> POST?
User -- useCreateAssistant--> POST?

Q: Can the shared logic be refactored out between Electron and Server implementations:

i.e.

Can this:
electron/handlers/fs.ts

  ipcMain.handle(
    'writeFile',
    async (event, path: string, data: string): Promise<void> => {
      try {
        await fs.writeFileSync(join(userSpacePath, path), data, 'utf8')
      } catch (err) {
        console.error(`writeFile ${path} result: ${err}`)
      }
    }
  )

Refactor into this:

infra/
    common/
        fs.ts
    electron/
        ___.ts/
    fastify-server/
        ___.ts/
    express-server/           // future possibility
        ... // import fs from common

Where Electron and Server share the same common/fs.ts implementation.

import writeFile from /common

// infra/electron/adapter/fs.ts (or whatever path/filename)
ipcMain.handle('writeFile', writeFile)

// infra/server/adapter/fs.ts (or whatever path/filename)
export async function writeFile() { writeFile }

Q: Can the api endpoints be auto generated? We're defining the entities anyway

// infra/server/index.ts
// One-time-macro to auto generate API routes e.g. `CRUD /createAssistant` from `/core`
// This means users can add additional endpoints via extensions (without explicit definition)

import assistant from janhq/core

fastify.createEndpointsFor(assistant)
@louis-jan
Copy link
Contributor

@linhtran174 is implementing this way

@freelerobot
Copy link
Contributor Author

Good stuff. Is it in fact less work/maintenance long term? OR more work?

@louis-jan
Copy link
Contributor

@0xSage less work/maintenance long term for sure

@dan-homebrew dan-homebrew modified the milestones: 0.4.0, 0.4.2, API Endpoint at localhost:1337, Jan Server, Jan has Clean Architecture Dec 11, 2023
@louis-jan
Copy link
Contributor

louis-jan commented Dec 12, 2023

Since #948, we even removed fs handler. IPC & REST routes are proxied automatically @0xSage

 Object.values(FileSystemRoute).forEach((route) => {
    ipcMain.handle(route, (event, ...args) => fs[route](...args)
  })

@freelerobot freelerobot moved this from In Review to Done in Jan & Cortex Dec 14, 2023
@freelerobot
Copy link
Contributor Author

Good stuff. Closing as this is addressed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P0: critical Mission critical
Projects
Archived in project
Development

No branches or pull requests

4 participants