-
-
Notifications
You must be signed in to change notification settings - Fork 3k
fix(create-astro): ensure that isEmpty converts dirPath to a string before checking its existence #14219
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
Conversation
🦋 Changeset detectedLatest commit: 04192ad The changes in this PR will be included in the next version bump. Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
…efore checking its existence Signed-off-by: Sebastian Beltran <[email protected]>
209a7ec to
68f551c
Compare
Signed-off-by: Sebastian Beltran <[email protected]>
delucis
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we know why dirPath isn’t a string here? We’re typing it as a string and presumably passing it something we think is a string, so I’m a bit surprised if it isn’t. What type were you seeing here?
i found that if dirPath is not a string or buffer, it will throw an error internally
Note that file URLs are also allowed (this is documented and you can see the conversion to string happening before they check validity).
Signed-off-by: Sebastian Beltran <[email protected]>
|
Basically, if the path isn’t passed as an argument in the console,
''.
It would be better to patch |
| ]; | ||
|
|
||
| export function isEmpty(dirPath: string) { | ||
| export function isEmpty(dirPath: string | undefined) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe a default parameter value would be sufficient here? ie
function isEmpty(dirPath: string = "")
| 'create-astro': patch | ||
| --- | ||
|
|
||
| Ensure that isEmpty converts dirPath to a string before checking its existence, since a future version of Node.js will throw an error. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The changeset is user-facing, so should tell users how it affects them, rather than the implementation. So here it would be something about it preventing it from logging a warning in Node 24+
|
I think rather than changing |
|
Hi @bjohansebas, are you still interested in this PR? |
|
Thanks for your help! |
Changes
After doing some more research, i found that if dirPath is not a string or buffer, it will throw an error internally (in https://github.com/nodejs/node/blob/9ec68afdc505215cd7b39c6fab35f11e3efa4fc7/lib/internal/fs/utils.js#L701). However, existsSync does not currently throw that error, it only issues a warning when that error code occurs ( https://github.com/nodejs/node/blob/9ec68afdc505215cd7b39c6fab35f11e3efa4fc7/lib/fs.js#L274).
In future versions of nodejs, if existsSync encounters that error code, it will throw an error. With this, our change should be fine.
The other uses of existsSync in Astro’s codebase already convert the path to a string using path.join or path.resolve, so this should be the only change needed to remove the warning.
closes: #14214
closes #14215
Testing
Docs