Skip to content

feat: track dependencies when loading config with native #19374

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

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

sapphi-red
Copy link
Member

@sapphi-red sapphi-red commented Feb 6, 2025

Description

reopen of #19350

Adds support for restarting the server when the dependency of the config file is edited.

I haven't wrote the tests yet.

refs #19178

@sapphi-red
Copy link
Member Author

One of the main concern for me about this is that this only works in node as the API isn't implemented in Bun or Deno iirc, and it's still experimental. In the past, I also thought about using tsx's tsImport within Vite, but the limitation held me back.

But maybe it's still worth including this for now though as the config loading feature is also experimental.

@bluwy
Yeah, it doesn't work with Bun / Deno. It's still experimental but it's also in a release candidate phase, the last phase before it is stablized. Given that it's a QOL improvement feature, I think the lack of support (for now) is fine (if we swap it in a major).

@sapphi-red
Copy link
Member Author

Writing a test for this is difficult. If I write a test in Vitest, the SSR transform replaces the dynamic import with the Vite's internal dynamic import function and the loader won't be called. To write a unit test that runs node command, I need to export the function but I don't want to export the function...

@bluwy
Copy link
Member

bluwy commented Feb 7, 2025

I'm personally fine without a test for now if it's difficult. I wonder if it's also good to extract this code as its own package too. I imagine other packages in the ecosystem may also find this useful, and it'll indirectly simplify testing. But that's just my thought 😅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants