fs: make `mkdtemp` accept buffers and URL by LiviaMedeiros · Pull Request #48828 · nodejs/node · GitHub

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

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

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')) {

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.

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.

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.

CI: https://ci.nodejs.org/job/node-test-pull-request/52876/

Rebased with 68703a6 for linter

Landed in 6cd6789...1eae568

PR-URL: nodejs#48828 Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>

PR-URL: #48828 Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>

PR-URL: nodejs#48828 Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>

PR-URL: #48828 Backport-PR-URL: #50669 Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>

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>

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

LiviaMedeiros force-pushed the fs-mkdtemp-url branch from 976f66b to f61da69 Compare July 18, 2023 16:05

LiviaMedeiros commented Jul 18, 2023

View reviewed changes

LiviaMedeiros added the request-ci Add this label to start a Jenkins CI on a PR. label Jul 18, 2023

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jul 18, 2023

This comment was marked as outdated.

Sign in to view

VoltrexKeyva approved these changes Jul 18, 2023

View reviewed changes

LiviaMedeiros added the request-ci Add this label to start a Jenkins CI on a PR. label Jul 18, 2023

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jul 18, 2023

This comment was marked as outdated.

Sign in to view

aduh95 reviewed Jul 19, 2023

View reviewed changes

Comment thread lib/internal/fs/utils.js Outdated

github-actions Bot mentioned this pull request Jul 20, 2023

CI Reliability 2023-07-20 nodejs/reliability#618

Open

22 tasks

LiviaMedeiros added the request-ci Add this label to start a Jenkins CI on a PR. label Jul 20, 2023

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jul 20, 2023

This comment was marked as outdated.

Sign in to view

aduh95 approved these changes Jul 20, 2023

View reviewed changes

github-actions Bot mentioned this pull request Jul 21, 2023

CI Reliability 2023-07-21 nodejs/reliability#619

Open

16 tasks

LiviaMedeiros force-pushed the fs-mkdtemp-url branch from cf63c24 to e589bd3 Compare July 21, 2023 15:50

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 force-pushed the fs-mkdtemp-url branch 2 times, most recently from 68703a6 to 85c0566 Compare July 21, 2023 16:06

LiviaMedeiros added the request-ci Add this label to start a Jenkins CI on a PR. label Jul 21, 2023

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jul 21, 2023

github-actions Bot mentioned this pull request Jul 27, 2023

CI Reliability 2023-07-27 nodejs/reliability#625

Open

18 tasks

Ceres6 pushed a commit to Ceres6/node that referenced this pull request Jul 27, 2023

fs: make mkdtemp accept buffers and URL

1f9ae42

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

fs: mention URL in NUL character error message

a7e3fa6

PR-URL: nodejs#48828 Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>

This was referenced Jul 28, 2023

CI Reliability 2023-07-28 nodejs/reliability#626

Open

CI Reliability 2023-07-29 nodejs/reliability#627

Open

CI Reliability 2023-07-30 nodejs/reliability#628

Open

Ceres6 pushed a commit to Ceres6/node that referenced this pull request Aug 14, 2023

fs: make mkdtemp accept buffers and URL

e014452

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

fs: mention URL in NUL character error message

b22843d

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

fs: make mkdtemp accept buffers and URL

6bd6214

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

fs: mention URL in NUL character error message

6252a3a

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

fs: make mkdtemp accept buffers and URL

bb4f60d

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

fs: make mkdtemp accept buffers and URL

f8b421e

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

fs: mention URL in NUL character error message

d2e92b9

PR-URL: #48828 Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>

UlisesGascon mentioned this pull request Aug 15, 2023

v20.6.0 proposal #49185

Merged

RafaelGSS pushed a commit that referenced this pull request Aug 17, 2023

fs: make mkdtemp accept buffers and URL

d634d78

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

fs: mention URL in NUL character error message

c14ff69

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

fs: make mkdtemp accept buffers and URL

4ffa98f

PR-URL: nodejs#48828 Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>

targos mentioned this pull request Nov 11, 2023

[v18.x] Backport most ESM and customization hook changes #50669

Closed

targos added backported-to-v18.x 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

fs: make mkdtemp accept buffers and URL

48c25b1

PR-URL: #48828 Backport-PR-URL: #50669 Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>

targos mentioned this pull request Nov 28, 2023

v18.19.0 release proposal #50953

Merged

fs: make `mkdtemp` accept buffers and URL by LiviaMedeiros · Pull Request #48828 · nodejs/node · GitHub

Uh oh!

Uh oh!

fs: make mkdtemp accept buffers and URL - #48828

fs: make mkdtemp accept buffers and URL#48828
LiviaMedeiros merged 2 commits into
nodejs:mainfrom
LiviaMedeiros:fs-mkdtemp-url

LiviaMedeiros commented Jul 18, 2023

Uh oh!

LiviaMedeiros Jul 18, 2023

Uh oh!

aduh95 Jul 19, 2023

Uh oh!

LiviaMedeiros Jul 19, 2023

Uh oh!

aduh95 Jul 19, 2023

Uh oh!

LiviaMedeiros Jul 20, 2023

Uh oh!

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

Uh oh!

This comment was marked as outdated.

nodejs-github-bot commented Jul 20, 2023

Uh oh!

LiviaMedeiros commented Jul 21, 2023

Uh oh!

LiviaMedeiros commented Jul 26, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

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

fs: make `mkdtemp` accept buffers and URL by LiviaMedeiros · Pull Request #48828 · nodejs/node · GitHub

Uh oh!

Uh oh!

fs: make mkdtemp accept buffers and URL - #48828

fs: make mkdtemp accept buffers and URL#48828LiviaMedeiros merged 2 commits intonodejs:mainnodejs/node:mainfrom LiviaMedeiros:fs-mkdtemp-urlLiviaMedeiros/node:fs-mkdtemp-urlCopy head branch name to clipboard

Conversation

LiviaMedeiros commented Jul 18, 2023

Uh oh!

LiviaMedeiros Jul 18, 2023

Choose a reason for hiding this comment

Uh oh!

aduh95 Jul 19, 2023

Choose a reason for hiding this comment

Uh oh!

LiviaMedeiros Jul 19, 2023

Choose a reason for hiding this comment

Uh oh!

aduh95 Jul 19, 2023

Choose a reason for hiding this comment

Uh oh!

LiviaMedeiros Jul 20, 2023

Choose a reason for hiding this comment

Uh oh!

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

Uh oh!

This comment was marked as outdated.

nodejs-github-bot commented Jul 20, 2023

Uh oh!

LiviaMedeiros commented Jul 21, 2023

Uh oh!

LiviaMedeiros commented Jul 26, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

fs: make mkdtemp accept buffers and URL#48828
LiviaMedeiros merged 2 commits into
nodejs:mainfrom
LiviaMedeiros:fs-mkdtemp-url