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): provide a separate logger interface #15631

Merged
merged 3 commits into from
Jan 19, 2024

Conversation

sheremet-va
Copy link
Member

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

Description

This PR standardizes the HMR logger interface.

  • This makes it easier to provide a custom logger because we expose only used methods.
  • During SSR the HMR update uses the full file path so the log is very long and not readable. This allows SSR logger to provide its own method to override the logged URL

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 17, 2024

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

patak-dev
patak-dev previously approved these changes Jan 17, 2024
@bluwy
Copy link
Member

bluwy commented Jan 18, 2024

This looks fine to me if it's staying as internal. If it's public one day, I'm concerned that we would have to add new methods if we want to log something structurally, and it'll break third-party implementations.

  • During SSR the HMR update uses the full file path so the log is very long and not readable. This allows SSR logger to provide its own method to override the logged URL

Before logging, is there a way we can prettify the paths first?

@sheremet-va
Copy link
Member Author

This looks fine to me if it's staying as internal. If it's public one day, I'm concerned that we would have to add new methods if we want to log something structurally, and it'll break third-party implementations.

It is a public API for SSR logger at least.

Before logging, is there a way we can prettify the paths first?

Before the logging we are actually making them longer 😄 Maybe if we change the format SSR runtime stores them in, then we won’t need this logger 🤔 I‘ll try to investigate this first before we can merge this.

@sheremet-va sheremet-va marked this pull request as draft January 18, 2024 06:55
@patak-dev
Copy link
Member

An option could be to provide a prettifyPath: (path: string) => string option instead of a logger and keep the rest as is.

@sheremet-va
Copy link
Member Author

sheremet-va commented Jan 18, 2024

@bluwy @patak-dev if we remove updated/invalidated do you think this would be a sufficient interface? Or would you prefer to keep console there? Or inherit Vite logger interface?

In vite-node PR this logger is a public API.


The second point of this PR is irrelevant now, I refactored how modules are stored to make it more inline with how browser/ssr worked before.

@bluwy
Copy link
Member

bluwy commented Jan 18, 2024

If we only have error and debug for now. I'm fine with that and we can expand later as needed 👍

@sheremet-va sheremet-va marked this pull request as ready for review January 18, 2024 16:23
@sheremet-va
Copy link
Member Author

Updated 😄 (not a big change, huh)

@patak-dev patak-dev merged commit 110e2e1 into vitejs:main Jan 19, 2024
10 checks passed
@sheremet-va sheremet-va deleted the feat/hmr-logger branch January 19, 2024 15:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants