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!: move side effect of restart server to the caller #8746

Merged
merged 5 commits into from
Oct 26, 2023

Conversation

CHOYSEN
Copy link
Contributor

@CHOYSEN CHOYSEN commented Jun 23, 2022

Description

make restart function more pure after expose it as a javascript API


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.
  • Read the Pull Request Guidelines and follow the Commit Convention.
  • 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).
  • Ideally, include relevant tests that fail without this PR but pass with it.

@netlify
Copy link

netlify bot commented Jun 23, 2022

Deploy Preview for vite-docs-main ready!

Name Link
🔨 Latest commit d63cdb3
🔍 Latest deploy log https://app.netlify.com/sites/vite-docs-main/deploys/62b494e7572cff0008f2a499
😎 Deploy Preview https://deploy-preview-8746--vite-docs-main.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@patak-dev
Copy link
Member

Would you explain why we would like to avoid the printing of the new URLs when calling restart(), but still have them in the HMR logic?

@CHOYSEN
Copy link
Contributor Author

CHOYSEN commented Jun 24, 2022

User may use Javascript API, createServer doesn't print the url automatically, so i think it shouldn't print when restarting either.

import { createServer } from "vite";
import { SHOULD_PRINT_URL } from "./consts";

const server = await createServer({ base: "/a/", configFile: false });
SHOULD_PRINT_URL && server.printUrls();

async function restartServer() {
  await server.restart();
  SHOULD_PRINT_URL && server.printUrls();
}

// ...
server.config.inlineConfig.base = "/b/";
restartServer();

@CHOYSEN
Copy link
Contributor Author

CHOYSEN commented Jul 4, 2022

The printing code was added without considering that the restart function would be exposed, but for now I think that code should be left to the caller.These prints are still necessary for Vite CLI-powered applications, so i left them in HMR logic. :)

@sapphi-red sapphi-red added p2-nice-to-have Not breaking anything but nice to have (priority) and removed p2-nice-to-have Not breaking anything but nice to have (priority) labels Jul 8, 2022
@sapphi-red
Copy link
Member

This makes sense to me partially.

But when metaframeworks that uses createServer and uses their own function similar to server.printUrls, they still cannot change the output when the config is changed.

We might need to think about #6997 (comment) and #8986 together with this PR.

@patak-dev
Copy link
Member

@CHOYSEN would you rebase this PR? Let's try to merge it for Vite 5, or otherwise decide if we prefer to keep things as is.

@patak-dev patak-dev added the p2-to-be-discussed Enhancement under consideration (priority) label Apr 6, 2023
@CHOYSEN
Copy link
Contributor Author

CHOYSEN commented Apr 7, 2023

sure

@sapphi-red sapphi-red added this to the 5.0 milestone May 3, 2023
@bluwy bluwy changed the title refactor: move side effect of restart server to the caller refactor!: move side effect of restart server to the caller Oct 23, 2023
bluwy
bluwy previously approved these changes Oct 23, 2023
ArnaudBarre
ArnaudBarre previously approved these changes Oct 23, 2023
@sapphi-red
Copy link
Member

/ecosystem-ci run

packages/vite/src/node/server/hmr.ts Outdated Show resolved Hide resolved
@vite-ecosystem-ci
Copy link

📝 Ran ecosystem CI on 00528b4: Open

suite result latest scheduled
analogjs success success
astro success success
histoire success success
ladle success success
laravel failure failure
marko failure failure
nuxt success success
nx failure failure
previewjs success success
qwik success success
rakkas success success
sveltekit success failure
unocss success success
vike success failure
vite-plugin-pwa failure failure
vite-plugin-react success success
vite-plugin-react-pages success success
vite-plugin-react-swc success success
vite-plugin-svelte success success
vite-plugin-vue success success
vite-setup-catalogue success success
vitepress success success
vitest success failure

@bluwy
Copy link
Member

bluwy commented Oct 24, 2023

The more I think about it, the more I'm unsure this is the right fix. This PR moves the printUrls away, but that still can be called in HMR. Personally I think the best way is this: "if printUrls was never called, then we also don't print it whenever we restart the server" 🤔

@bluwy bluwy dismissed stale reviews from ArnaudBarre and themself via ec2c0ce October 25, 2023 04:50
@bluwy
Copy link
Member

bluwy commented Oct 25, 2023

Added the printing for shortcuts, but I'm still not sure if this is the right solution.

patak-dev
patak-dev previously approved these changes Oct 26, 2023
Copy link
Member

@patak-dev patak-dev left a comment

Choose a reason for hiding this comment

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

I think your latest changes looks good @bluwy, let's go with this approach

@patak-dev
Copy link
Member

Oh, there is a conflict, I think you can merge it once it is resolved then.

@patak-dev patak-dev modified the milestones: 5.0, 6.0 Oct 26, 2023
@bluwy bluwy modified the milestones: 6.0, 5.0 Oct 26, 2023
@bluwy bluwy merged commit 521ca58 into vitejs:main Oct 26, 2023
9 of 10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change p2-to-be-discussed Enhancement under consideration (priority)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants