fix(unplugin): do not let Vite dev polling timer keep the process alive - #1846
fix(unplugin): do not let Vite dev polling timer keep the process alive#1846HsukqiLee wants to merge 2 commits into
Conversation
Signed-off-by: Hsukqi Lee <team@tsinbei.com>
|
@HsukqiLee is attempting to deploy a commit to the Meta Open Source Team on Vercel. A member of the Team first needs to authorize it. |
|
Hi @HsukqiLee! Thank you for your pull request and welcome to our community. Action RequiredIn order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you. ProcessIn order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA. Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with If you have received this in error or have any questions, please contact us at cla@meta.com. Thanks! |
There was a problem hiding this comment.
🟡 Changes recommended
One of the new regression tests doesn’t actually assert that the interval is cleared on close, so it could pass even if the cleanup regressed.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Fixes a Vite dev-server timer leak in @stylexjs/unplugin that can keep the Node event loop alive under Vitest (where server.httpServer is absent), causing vitest run to hang on shutdown.
Changes:
viteadapter:unref()the shared-store pollingsetIntervalso it can’t block process exit, while keeping existinghttpServer.closecleanup.- Expose the polling interval on the Vite server instance (
__stylexSharedPollingInterval) for test/host cleanup. - Add regression tests covering
unref()behavior and close cleanup wiring.
File summaries
| File | Description |
|---|---|
| packages/@stylexjs/unplugin/src/vite.js | Unrefs the shared-store polling interval and exposes it on the server for observability/cleanup. |
| packages/@stylexjs/unplugin/tests/unplugin.test.js | Adds regression tests for the polling interval behavior in Vitest/no-httpServer and close-cleanup scenarios. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| test('is cleared when the httpServer closes', () => { | ||
| const plugin = unplugin.vite({ dev: true }); | ||
| const closeListeners = []; | ||
| const httpServer = { | ||
| once(event, fn) { | ||
| if (event === 'close') closeListeners.push(fn); | ||
| }, | ||
| }; | ||
| const server = makeServer(httpServer); | ||
| plugin.configureServer(server); | ||
|
|
||
| const interval = server.__stylexSharedPollingInterval; | ||
| expect(interval).toBeDefined(); | ||
| expect(closeListeners.length).toBe(1); | ||
| // Simulate the server closing. | ||
| closeListeners[0](); | ||
| // After close the timer is cleared: calling clearInterval again is safe, | ||
| // and the interval no longer has a ref either way. | ||
| expect(() => clearInterval(interval)).not.toThrow(); | ||
| }); |
There was a problem hiding this comment.
Good catch — rewritten in 91b2507 with fake timers and a ws.send spy: bumps the shared store version, asserts one stylex:css-update send on the next tick, then simulates close and asserts no further sends after another version bump.
meta-cla
Bot
added
the
CLA Signed
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks! |
Signed-off-by: Hsukqi Lee <team@tsinbei.com>
What changed / motivation ?
@stylexjs/unplugin's Vite adapter keeps Node alive after tests finish when used under Vitest. InconfigureServer, when the shared store is active the plugin starts a 150 mssetIntervalto broadcaststylex:css-update, but clears it only viaserver.httpServer?.once('close', ...). Vitest's Vite server has nohttpServer, so the cleanup listener is skipped and the interval is never cleared — everyvitest runends withclose timed out after 10000msand suite wall time jumps from ~4.5 s to ~15 s (#1836).This change calls
interval.unref()so the dev-only polling timer can never hold the event loop open, while keeping the existinghttpServerclosecleanup for normal dev servers. The interval is also exposed on the server (__stylexSharedPollingInterval) so hosts that tear down a server without anhttpServerclose event (and tests) can observe/clear it. Behavior is unchanged for real dev servers (the timer still fires and is cleared on close); it only stops blocking process exit when there is nohttpServer.Linked PR/Issues
Fixes #1836
Additional Context
__tests__/unplugin.test.js:unref'd (hasRef() === false) when the server has nohttpServer(the Vitest case);httpServeremitsclose.interval.unref?.()so a leaked timer cannot keep Node alive.Pre-flight checklist
Contribution Guidelines