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

refactor(hmr): keep buffer implementation internal, expose queueUpdate #15486

Merged
merged 8 commits into from
Jan 11, 2024

Conversation

sheremet-va
Copy link
Member

@sheremet-va sheremet-va commented Jan 2, 2024

When implementing #12165, I noticed a lot of repetition that I thought might be an architectural problem. I am creating a separate PR to make it easier to review.

Description

Additional context


What is the purpose of this pull request?

  • Bug fix
  • New Feature
  • Documentation update
  • Other

Before submitting the PR, please make sure you do the following

  • Read the Contributing Guidelines, especially the Pull Request Guidelines.
  • Check that there isn't already a PR that solves the problem the same way to avoid creating a duplicate.
  • Provide a description in this PR that addresses what the PR is solving, or reference the issue that it solves (e.g. fixes #123).
  • Update the corresponding documentation if needed.
  • Ideally, include relevant tests that fail without this PR but pass with it.

Copy link

stackblitz bot commented Jan 2, 2024

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

Comment on lines 210 to 219
public addBuffer(message: string): void {
this.buffer.push(message)
}

public sendCustomMessages(): void {
if (this.connection.isReady()) {
this.buffer.forEach((msg) => this.connection.send(msg))
this.buffer = []
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Should this be named something like queue(message) and flush()? And maybe also expose a send(message) that does a queue(message); flush()

Copy link
Member Author

@sheremet-va sheremet-va Jan 8, 2024

Choose a reason for hiding this comment

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

From the naming side, they definitely need to have more words because HMR client is about handling module reloading as a whole, not just about communication. So I am fine with queueMessage (especially because we have queueUpdate), but I am not sure what is the purpose of a separate flush method

Copy link
Member

Choose a reason for hiding this comment

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

I mean renaming sendCustomMessages() to flush() (or flushMessages()). As in https://nodejs.org/api/http.html#requestflushheaders. Not having an extra method. The extra would be sendMessages() that does a queueMessage() then flushMessages(). For example, here you have:

    this.hmrClient.addBuffer(JSON.stringify({ type: 'custom', event, data }))
    this.hmrClient.sendCustomMessages()

that could be

    this.hmrClient.sendMessage(JSON.stringify({ type: 'custom', event, data }))

if we have the three methods (sendMessage, queueMessage, flushMessages)

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated again 😄

@patak-dev patak-dev added the p3-significant High priority enhancement (priority) label Jan 9, 2024
Copy link
Member

@bluwy bluwy left a comment

Choose a reason for hiding this comment

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

Looks like a clean refactor!

@patak-dev patak-dev merged commit c029efb into vitejs:main Jan 11, 2024
10 checks passed
@sheremet-va sheremet-va deleted the refactor/hmr-buffer branch January 19, 2024 12:03
@sheremet-va sheremet-va restored the refactor/hmr-buffer branch January 19, 2024 12:03
@sheremet-va sheremet-va deleted the refactor/hmr-buffer branch January 19, 2024 12:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p3-significant High priority enhancement (priority)
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants