Skip to content

Conversation

@avivkeller
Copy link
Member

Currently, doc-kit does not work on Windows due to a Rolldown issue, and a paths issue.

This PR fixes the paths issue by using Windows-safe paths.

Additionally,

  • When not minified, there is a chance that the code will end with an //#endregion comment. We need to add a newline before the return
  • We set the variable code = to to dehydration, not our generated variable. TBH, I'm not sure how this worked at all previously...

@TheAlexLichter There appears to be a bug when building this on Windows:
It appears to import everything twice. Once using a Unix path, and once using a Windows path.

Copilot AI review requested due to automatic review settings July 19, 2025 22:02
@avivkeller avivkeller requested a review from a team as a code owner July 19, 2025 22:02
@vercel
Copy link

vercel bot commented Jul 19, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
api-docs-tooling ✅ Ready (Inspect) Visit Preview Jul 19, 2025 10:02pm

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR fixes Windows compatibility issues in doc-kit by replacing new URL().pathname with Windows-safe path resolution using POSIX paths. The changes also correct variable naming inconsistencies and improve code generation formatting.

  • Introduces Windows-safe path handling using node:path/posix and a computed ROOT constant
  • Fixes variable naming bug where server-side rendering used incorrect variable references
  • Updates external dependencies configuration and improves code formatting

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

File Description
src/generators/web/constants.mjs Adds ROOT constant calculation and replaces URL.pathname with resolve() calls for Windows compatibility
src/generators/web/utils/generate.mjs Updates import path resolution and fixes variable naming in server program generation
src/generators/web/utils/processing.mjs Moves SSR variable declaration to module scope and fixes variable reference bug
src/generators/web/utils/bundle.mjs Adds 'preact-render-to-string' to external dependencies list

@TheAlexLichter
Copy link

When not minified, there is a chance that the code will end with an //#endregion comment. We need to add a newline before the return

You can disable region comments via experimental.attachDebugData (rolldown/rolldown#4918)

@TheAlexLichter There appears to be a bug when building this on Windows: It appears to import everything twice. Once using a Unix path, and once using a Windows path.

Oh 👀
Would appreciate a minimal reproduction & issue in the https://github.com/rolldown/rolldown/ repo if possible 🙏🏻

@avivkeller
Copy link
Member Author

Would appreciate a minimal reproduction & issue in the https://github.com/rolldown/rolldown/ repo if possible 🙏🏻

Will do! I'll work on getting a minimal repro of this, and the other issue we spoke about, today.

@avivkeller
Copy link
Member Author

@nodejs/web-infra requesting fast track, as this is used in a reproduction for Rolldown.

@avivkeller avivkeller added the fast track This PR can land before the typical review time, with a :+1: from collaborators label Jul 20, 2025
@avivkeller avivkeller merged commit cbc2a99 into main Jul 20, 2025
17 checks passed
@avivkeller avivkeller deleted the fix/windows-safe branch July 20, 2025 18:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

fast track This PR can land before the typical review time, with a :+1: from collaborators

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants