fix: windows gen file #970
Conversation
|
Someone is attempting to deploy a commit to the TypeCell Team on Vercel. A member of the Team first needs to authorize it. |
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
Good progress 👍 I just tested it on both Windows 10 and MacOS and here are the current results
By "adds example files" I mean the |
|
@jkcs I think the only part missing is in the Update: |
|
Thank you for the detailed testing you have done, now lets try again |
|
I just tested it again and everything seems to be working as expected in both Windows 10 and MacOS 👍
@jkcs Thanks for your work 👍 |
packages/dev-scripts/examples/gen.ts
Outdated
| @@ -57,7 +58,7 @@ async function writeTemplate(project: Project, templateFile: string) { | |||
|
|
|||
| async function generateCodeForExample(project: Project) { | |||
| const templates = glob.sync( | |||
| path.resolve(dir, "./template-react/*.template.tsx") | |||
| path.resolve(dir, "./template-react/*.template.tsx").replace(/\\/g, "/") | |||
There was a problem hiding this comment.
instead of replace, using path.join is cleaner?
There was a problem hiding this comment.
path.join cannot solve the problem here, in fact, it's because path.sep used in different systems is different
There was a problem hiding this comment.
ah, and the is that glob always expects a fwd slash I suppose?
| @@ -114,7 +116,7 @@ export type Files = Record< | |||
|
|
|||
| export function getProjectFiles(project: Project): Files { | |||
| const dir = path.resolve("../../", project.pathFromRoot); | |||
| const files = glob.globSync(dir + "/**/*", { | |||
| const files = glob.globSync(glob.win32.convertPathToPattern(dir + "/**/*"), { | |||
There was a problem hiding this comment.
Now we're also calling win32.convertPathToPattern on mac / unix right? Does that make sense? It seems a bit weird to me!
There was a problem hiding this comment.
In fact, it just does the same thing as above, turning \ into /. The reason we don't use glob.convertPathToPattern here is that it doesn't work on Windows, which seems a bit weird.
| @@ -140,7 +142,11 @@ export function getProjectFiles(project: Project): Files { | |||
| */ | |||
| export function getExampleProjects(): Project[] { | |||
| const examples: Project[] = glob | |||
| .globSync(path.join(dir, "../../../examples/**/*/.bnexample.json")) | |||
| .globSync( | |||
| glob.win32.convertPathToPattern( | |||
| pathFromRoot: path.relative( | ||
| path.resolve("../../"), | ||
| path.join(directory, "..") | ||
| pathFromRoot: slash( |
There was a problem hiding this comment.
I'm not sure slash is needed (would prefer not to add this dependency). maybe we can just prevent passing / to path.resolve? e.g.: path.resolve("..","..")
There was a problem hiding this comment.
Can we define a normalizeToPOSIX function to handle this uniformly? Preventing passing / to path.resolve does not work
There was a problem hiding this comment.
Yes, I'd love to see this code a bit clearer / more uniform. When reviewing, it's not entirely clear to me why we have 3 different methods:
replaceslashconvertPathToPattern
Can we either use 1 method (ideally with a separate, well named helper function). Or, if multiple methods are necessary, add a comment in code why we use each function?
|
@YousefED Take another look |
| @@ -201,3 +202,13 @@ export function getExampleProjects(): Project[] { | |||
| // }); | |||
| return examples; | |||
| } | |||
|
|
|||
| export function replacePathSepToSlash(path: string) { | |||
| const isExtendedLengthPath = path.startsWith("\\\\?\\"); | |||
There was a problem hiding this comment.
what's an example of this?
There was a problem hiding this comment.
From here
Forward-slash paths can be used in Windows as long as they're not extended-length paths.
/claim #955
close #955