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

feat(108): Stream Request and Response Bodies #116

Merged
merged 8 commits into from
Nov 8, 2024

Conversation

SoulKa
Copy link
Collaborator

@SoulKa SoulKa commented Oct 31, 2024

Changes

  • Use latest LTS of node, which is Node22
  • Create IpcPushStream class in renderer, which receives stream data pushed to the renderer (a pull stream would be a stream that supports read())
  • Create event handler in main process that opens streams and sends their data chunk by chunk to renderer
  • Remove unused event handler methods for reading files
  • Fix binding of ipcRenderer methods in context bridge
  • Implement abstract EventEmitter class in renderer
  • Fix switching requests and losing changes of request body

Testing

  • Switch between requests and see the request body being loaded
  • Send request and see response being loaded

Checklist

  • Issue has been linked to this PR
  • Code has been reviewed by person creating the PR
  • Commit messages, branch names, code formatting adheres to our Contributing Guidelines
  • Automated tests have been written, if possible
  • Manual testing has been performed
  • Documentation has been updated, if necessary
  • Changes have been reviewed by second person

@SoulKa SoulKa added enhancement New feature or request frontend Frontend related task backend Backend related task labels Oct 31, 2024
@SoulKa SoulKa self-assigned this Oct 31, 2024
@SoulKa SoulKa linked an issue Oct 31, 2024 that may be closed by this pull request
Copy link
Collaborator

@kwerber kwerber left a comment

Choose a reason for hiding this comment

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

What is the reason that we have not used main-event-service for the stream events? Do we need to have "lower level access" for performance or code readability reasons? Is the main-event-service abstraction unfit for this purpose?

src/main/event/stream-events.ts Show resolved Hide resolved
@SoulKa
Copy link
Collaborator Author

SoulKa commented Nov 8, 2024

What is the reason that we have not used main-event-service for the stream events? Do we need to have "lower level access" for performance or code readability reasons? Is the main-event-service abstraction unfit for this purpose?

The main-event-service only supports ipcMain.handle() for an equivalent invoke() on the renderer side. But I also need ipcMain.on(). Also, the main event handler has only "public" methods. I do not want to expose the stream.close() method globally. This should only be called by the stream internally.

@SoulKa SoulKa merged commit 205e4f4 into main Nov 8, 2024
2 checks passed
@SoulKa SoulKa deleted the feature/108-request-body-stream branch November 8, 2024 09:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend Backend related task enhancement New feature or request frontend Frontend related task
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support loading Request Bodies as Stream
2 participants