[docs-infra] Fix windows support #920
Conversation
dav-is
added
type: bug
Bundle size report
Check out the code infra dashboard for more information about this PR. |
| webpack: (config, { buildId, dev, isServer, defaultLoaders, webpack }) => { | ||
| config.module.rules.push({ | ||
| test: /\/demos\/[^/]+\/client\.ts$/, | ||
| test: /[/\\]demos[/\\][^/\\]+[/\\]client\.ts$/, |
There was a problem hiding this comment.
shouldn't be necessary, webpack normalizes paths to posix
There was a problem hiding this comment.
I thought so too, but it seemed to make a difference when Zeeshan tested it
…/mui-public into davis/fix-docs-infra-windows-support
| * which only supports POSIX paths. The key insight is that by stripping the `file://` | ||
| * prefix and normalizing backslashes to forward slashes, we get a path that: | ||
| * - On Unix: `/home/user/file.ts` - works directly with path-module | ||
| * - On Windows: `/C:/Users/file.ts` - also works with path-module because it starts with `/` |
There was a problem hiding this comment.
Don't we use path-module only as a browser shim? It feels wrong to me that it would ever receive absolute paths witha drive letter, regardless of the platform. I'd expect them to be project-relative at least.
There was a problem hiding this comment.
As we discussed, the intention is to preserve the value of import.meta.url as long as possible because it is fairly portable. Then if we need to transform the path, we can convert it to a portable POSIX path, then back into a file:// URL when finished. If we need to modify the filesystem, we can use the Node.js url module to convert it into a compatible Windows path.
path-module only has POSIX support so fails with native windows paths. We can normalize the paths and still use the isomorphic path-module.
If this code runs on the client side, it would be receiving an https:// URL and transforming its path.
oliviertassinari
added
the
type: regression
|
Adding #948, after merging we can enable windows on this PR to prove it works |
github-actions
bot
added
the
PR: out-of-date
github-actions
bot
removed
the
PR: out-of-date
Thanks, merged your change and enabled windows and it's working with this PR now |
We should consider the case where Windows returns
\as the path separator andC:/without a leading slash. We can use thenode:urlmodule to convert from the URL to a path when accessing the filesystem. When we want to analyze and transform the URL, we can normalize it.file:///C:\is how paths on Windows start. If you runnew URL('file:///C:\').pathname, it returns/C:\with the leading slash, invalid in the filesystem, but compatible with POSIX path transforms after replacing every\. We can do this without thenode:urlmodule by simply removing thefile://. This also follows the pattern that each factory has its own distinct URL and makes logs more useful.