Skip to content
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

fix(gatsby): ensure that writing node manifests to disk does not break on Windows #33853

Merged
merged 7 commits into from
Nov 12, 2021

Conversation

veryspry
Copy link
Contributor

@veryspry veryspry commented Nov 4, 2021

Description

Writing node manifest files is failing on Windows and this provides a fix for that.

Related Issues

Resolves #33743

@gatsbot gatsbot bot added the status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer label Nov 4, 2021
@@ -239,14 +239,28 @@ export async function processNodeManifest(
const gatsbySiteDirectory = store.getState().program.directory

// write out the manifest file
const manifestFilePath = path.join(
let manifestFilePath = path.join(
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

It doesn't look like it handles :'s

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm down but I'm curious, what does this quadruple backslash do for us?

Copy link
Contributor Author

@veryspry veryspry Nov 4, 2021

Choose a reason for hiding this comment

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

Oh yeah, also it won't solve this issue on its own (doesn't handle : like @TylerBarnes said). But if it gives us some extra guardrails for other scenarios using it makes sense

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, got it, @KyleAMathews I thought you meant can we use it to solve this problem. I see you mean for extra edge-case guards like @veryspry is saying. Makes sense!

Copy link
Contributor

Choose a reason for hiding this comment

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

sorry I should have been more direct — this is the utility we use all over core for creating cross-os paths (https://github.com/gatsbyjs/gatsby/search?q=joinPath) — I forget its whole history but it should be pretty battle-tested now.

I see now better what you're solving e.g. that some manifest IDs can have reserved characters in them. Wouldn't we always need to replace those? There's some others we'd need to handle https://superuser.com/questions/358855/what-characters-are-safe-in-cross-platform-file-names-for-linux-windows-and-os

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Gotcha! I'll make that change

Yes, we probably do need to handle more characters other than the colon. : is probably the most pressing since a node manifest file name will likely be created using a timestamp but I'll look into handling other cases as well,

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I made the change to use joinPath like you suggested.

Also added some more chars to the replace regex to account for other special chars on Windows

@LekoArts LekoArts changed the title fix: ensure that writing node manifests to disk does not break on Windows fix(gatsby): ensure that writing node manifests to disk does not break on Windows Nov 4, 2021
@LekoArts LekoArts added topic: core Relates to Gatsby's core (e.g. page loading, reporter, state machine) and removed status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer labels Nov 4, 2021
@veryspry veryspry force-pushed the veryspry/fix-windows-node-manifest branch from 749738c to 57a733d Compare November 4, 2021 18:52
KyleAMathews
KyleAMathews previously approved these changes Nov 4, 2021
Copy link
Contributor

@KyleAMathews KyleAMathews left a comment

Choose a reason for hiding this comment

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

:shipit:

Copy link
Contributor

@TylerBarnes TylerBarnes left a comment

Choose a reason for hiding this comment

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

Heck yes! 💪

@TylerBarnes TylerBarnes self-requested a review November 4, 2021 21:57
Copy link
Contributor

@TylerBarnes TylerBarnes left a comment

Choose a reason for hiding this comment

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

Looks like the processNodeManifest tests need to be updated to pass on windows

@TylerBarnes
Copy link
Contributor

I wonder if we shouldn't use joinPath here 🤔 the result in the tests seems like too many backslashes

Expected: "C:\\Users\\circleci\\project\\public\\__node-manifests\\test\\1.json", ObjectContaining {"page": {"path": "/1"}}
Received
->     1: "C:\\\\Users\\\\circleci\\\\project\\\\public\\\\__node-manifests\\\\test\\\\1.json"

I might be missing something though

@veryspry
Copy link
Contributor Author

veryspry commented Nov 4, 2021

I tested on a Windows machine with this canary and node manifests were written correctly to disk

gatsby@3.10.0-alpha-node-manifest-windows-escape.30+cd4571cfd3

@TylerBarnes
Copy link
Contributor

Sweet 😄 I guess we just need to use joinPath in our unit tests then

@veryspry
Copy link
Contributor Author

veryspry commented Nov 4, 2021

I tested on a Windows machine with this canary and node manifests were written correctly to disk

gatsby@3.10.0-alpha-node-manifest-windows-escape.30+cd4571cfd3

Oh NVM, I did not confirm that conclusively. Looks like there was a webpack error during build time that must have prevented the files from getting written to disk? The log saying that the files were successfully written running though 🤔

I need to head out for the night but I will dig in some more in the morning

@veryspry
Copy link
Contributor Author

veryspry commented Nov 8, 2021

I wonder if we shouldn't use joinPath here thinking the result in the tests seems like too many backslashes

Expected: "C:\\Users\\circleci\\project\\public\\__node-manifests\\test\\1.json", ObjectContaining {"page": {"path": "/1"}}
Received
->     1: "C:\\\\Users\\\\circleci\\\\project\\\\public\\\\__node-manifests\\\\test\\\\1.json"

I might be missing something though

I'm pretty sure that this is okay since the extra backslashes should be ignored as redundant escape chars. I'm guessing the use of four \ in a row in joinPath is to ensure that the Windows path delimiter is escaped properly inside interpolated strings

@wardpeet
Copy link
Contributor

wardpeet commented Nov 8, 2021

joinPath is only necessary when you use it in a string, here it's actually useless. I wonder why we don't use @sindresorhus/slugify to remove all special characters.

@veryspry
Copy link
Contributor Author

veryspry commented Nov 9, 2021

joinPath is only necessary when you use it in a string, here it's actually useless. I wonder why we don't use @sindresorhus/slugify to remove all special characters.

@wardpeet Aha, that makes sense. I'm going to revert it then in favor of path.join. I suppose that @sindresorhus/slugify would make sense since it handles characters other than then ones we included in this check. The only question I have is, does this handle backslashes and other characters that are specifically reserved on Windows? It is unclear from the package README if it does

On another note, we really should only be running the replace function on the filename, not the whole path. That way we can also replace any instances of \ before building the the full path

@wardpeet
Copy link
Contributor

wardpeet commented Nov 9, 2021

Oh yes only on the filename not full path 👍. This more or less removes os specific code

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic: core Relates to Gatsby's core (e.g. page loading, reporter, state machine)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Node manifest file paths aren't created correctly
6 participants