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

fs: make mkdtemp accept buffers and URL #48828

Merged
merged 2 commits into from
Jul 26, 2023

Conversation

LiviaMedeiros
Copy link
Contributor

mkdtemp() seems to be the only fs method that doesn't support URL objects yet.

@nodejs-github-bot nodejs-github-bot added fs Issues and PRs related to the fs subsystem / file system. needs-ci PRs that need a full CI run. labels Jul 18, 2023
@@ -749,7 +749,7 @@ let nonPortableTemplateWarn = true;
function warnOnNonPortableTemplate(template) {
// Template strings passed to the mkdtemp() family of functions should not
// end with 'X' because they are handled inconsistently across platforms.
if (nonPortableTemplateWarn && StringPrototypeEndsWith(template, 'X')) {
if (nonPortableTemplateWarn && StringPrototypeEndsWith(`${template}`, 'X')) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

String.prototype.endsWith always strigifies this value but explicit coercion wouldn't hurt

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would be a mistake to attempt to stringify a TypedArray, let's not bother with this warning if the template is not a string, wdyt?

Suggested change
if (nonPortableTemplateWarn && StringPrototypeEndsWith(`${template}`, 'X')) {
if (nonPortableTemplateWarn && typeof template === 'string' && StringPrototypeEndsWith(template, 'X')) {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh that's right, it can be native Uint8Array, too. Fixed that in the actual functions. 😅

As for warning, AFAICT there's no utf character that ends with 0x58 except for one-byte X, so Array.prototype.at.call(template, -1) === 0x58 should suffice for buffers.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure it's an always correct assumption that the system would be using a UTF encoding set, but maybe it's fair to consider so as it's certainly very common 🤔 but probably folks who are using a Buffer/Uint8Array are doing so precisely so it can work on weird encoding sets? Maybe it's not worth overthinking it, it's just emitting a warning, so it's not like it would block anyone to have false positive.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The warning makes sense because the underlying implementation still might get confused.
glibc performs multibyte-unfriendly check with strspn, and it might be likely for mkdtemp on exotic platforms supporting prefixes longer than 6 to be as rough as for (int i = strlen(template); template[--i] == 'X'; template[i] = getRandomChar()) {}.
IMHO if someone writes precise code for specific platforms and charsets, it's probably up to them to simply ignore warning about template not being portable.

@LiviaMedeiros LiviaMedeiros added the request-ci Add this label to start a Jenkins CI on a PR. label Jul 18, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jul 18, 2023
@nodejs-github-bot

This comment was marked as outdated.

@LiviaMedeiros LiviaMedeiros added the request-ci Add this label to start a Jenkins CI on a PR. label Jul 18, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jul 18, 2023
@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

lib/internal/fs/utils.js Outdated Show resolved Hide resolved
@LiviaMedeiros LiviaMedeiros added the request-ci Add this label to start a Jenkins CI on a PR. label Jul 20, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jul 20, 2023
@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot
Copy link
Collaborator

@LiviaMedeiros LiviaMedeiros added request-ci Add this label to start a Jenkins CI on a PR. commit-queue-rebase Add this label to allow the Commit Queue to land a PR in several commits. and removed request-ci Add this label to start a Jenkins CI on a PR. labels Jul 21, 2023
@LiviaMedeiros LiviaMedeiros force-pushed the fs-mkdtemp-url branch 2 times, most recently from 68703a6 to 85c0566 Compare July 21, 2023 16:06
@LiviaMedeiros
Copy link
Contributor Author

Rebased with 68703a6 for linter

@LiviaMedeiros LiviaMedeiros added the request-ci Add this label to start a Jenkins CI on a PR. label Jul 21, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jul 21, 2023
@LiviaMedeiros
Copy link
Contributor Author

Landed in 6cd6789...1eae568

Ceres6 pushed a commit to Ceres6/node that referenced this pull request Jul 27, 2023
PR-URL: nodejs#48828
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Jul 27, 2023
PR-URL: nodejs#48828
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
pluris pushed a commit to pluris/node that referenced this pull request Aug 6, 2023
PR-URL: nodejs#48828
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
pluris pushed a commit to pluris/node that referenced this pull request Aug 6, 2023
PR-URL: nodejs#48828
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
pluris pushed a commit to pluris/node that referenced this pull request Aug 7, 2023
PR-URL: nodejs#48828
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
pluris pushed a commit to pluris/node that referenced this pull request Aug 7, 2023
PR-URL: nodejs#48828
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Aug 14, 2023
PR-URL: nodejs#48828
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Aug 14, 2023
PR-URL: nodejs#48828
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Aug 14, 2023
PR-URL: nodejs#48828
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Aug 14, 2023
PR-URL: nodejs#48828
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
UlisesGascon pushed a commit to UlisesGascon/node that referenced this pull request Aug 14, 2023
PR-URL: nodejs#48828
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
UlisesGascon pushed a commit to UlisesGascon/node that referenced this pull request Aug 14, 2023
PR-URL: nodejs#48828
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
RafaelGSS pushed a commit that referenced this pull request Aug 15, 2023
PR-URL: #48828
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
RafaelGSS pushed a commit that referenced this pull request Aug 15, 2023
PR-URL: #48828
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
@UlisesGascon UlisesGascon mentioned this pull request Aug 15, 2023
RafaelGSS pushed a commit that referenced this pull request Aug 17, 2023
PR-URL: #48828
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
RafaelGSS pushed a commit that referenced this pull request Aug 17, 2023
PR-URL: #48828
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
targos pushed a commit to targos/node that referenced this pull request Nov 11, 2023
PR-URL: nodejs#48828
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
@targos targos added backported-to-v18.x PRs backported to the v18.x-staging branch. and removed commit-queue-failed An error occurred while landing this pull request using GitHub Actions. labels Nov 23, 2023
targos pushed a commit that referenced this pull request Nov 23, 2023
PR-URL: #48828
Backport-PR-URL: #50669
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
sercher added a commit to sercher/graaljs that referenced this pull request Apr 25, 2024
PR-URL: nodejs/node#48828
Backport-PR-URL: nodejs/node#50669
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
sercher added a commit to sercher/graaljs that referenced this pull request Apr 25, 2024
PR-URL: nodejs/node#48828
Backport-PR-URL: nodejs/node#50669
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backported-to-v18.x PRs backported to the v18.x-staging branch. commit-queue-rebase Add this label to allow the Commit Queue to land a PR in several commits. fs Issues and PRs related to the fs subsystem / file system. needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants