fs: make `mkdtemp` accept buffers and URL#48828

LiviaMedeiros · 2023-07-18T16:03:09Z

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

LiviaMedeiros · 2023-07-18T16:13:02Z

lib/internal/fs/utils.js

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


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&&typeoftemplate==='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.

lib/internal/fs/utils.js

nodejs-github-bot · 2023-07-20T19:03:21Z

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

LiviaMedeiros · 2023-07-21T16:12:23Z

Rebased with 68703a6 for linter

LiviaMedeiros · 2023-07-26T11:39:31Z

Landed in 6cd6789...1eae568

PR-URL: nodejs#48828 Reviewed-By: Mohammed Keyvanzadeh <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]>

PR-URL: #48828 Reviewed-By: Mohammed Keyvanzadeh <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]>

PR-URL: nodejs#48828 Reviewed-By: Mohammed Keyvanzadeh <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]>

PR-URL: #48828 Backport-PR-URL: #50669 Reviewed-By: Mohammed Keyvanzadeh <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]>

PR-URL: nodejs/node#48828 Backport-PR-URL: nodejs/node#50669 Reviewed-By: Mohammed Keyvanzadeh <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]>

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 f61da69Compare 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-actionsbot 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-actionsbot 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

lib/internal/fs/utils.js Outdated Show resolvedHide resolved

github-actionsbot 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-actionsbot 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-actionsbot 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 e589bd3Compare 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 85c0566Compare 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-actionsbot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jul 21, 2023

github-actionsbot 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 <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]>

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 <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]>

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

pluris pushed a commit to pluris/node that referenced this pull request Aug 6, 2023
fs: make mkdtemp accept buffers and URL
2190c11
PR-URL: nodejs#48828 Reviewed-By: Mohammed Keyvanzadeh <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]>

pluris pushed a commit to pluris/node that referenced this pull request Aug 6, 2023
fs: mention URL in NUL character error message
4ab3200
PR-URL: nodejs#48828 Reviewed-By: Mohammed Keyvanzadeh <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]>

pluris pushed a commit to pluris/node that referenced this pull request Aug 7, 2023
fs: make mkdtemp accept buffers and URL
69980c7
PR-URL: nodejs#48828 Reviewed-By: Mohammed Keyvanzadeh <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]>

pluris pushed a commit to pluris/node that referenced this pull request Aug 7, 2023
fs: mention URL in NUL character error message
e85b6bc
PR-URL: nodejs#48828 Reviewed-By: Mohammed Keyvanzadeh <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]>

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 <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]>

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 <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]>

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 <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]>

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 <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]>

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 <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]>

UlisesGascon pushed a commit to UlisesGascon/node that referenced this pull request Aug 14, 2023
fs: mention URL in NUL character error message
5352f41
PR-URL: nodejs#48828 Reviewed-By: Mohammed Keyvanzadeh <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]>

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 <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]>

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 <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]>

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 <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]>

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 <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]>

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 <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]>

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 <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]>

targos mentioned this pull request Nov 28, 2023
v18.19.0 release proposal #50953
Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

fs: make `mkdtemp` accept buffers and URL#48828

fs: make `mkdtemp` accept buffers and URL #48828

Uh oh!

LiviaMedeiros commented Jul 18, 2023

Uh oh!

LiviaMedeirosJul 18, 2023

Uh oh!

aduh95Jul 19, 2023

Uh oh!

LiviaMedeirosJul 19, 2023

Uh oh!

aduh95Jul 19, 2023

Uh oh!

LiviaMedeirosJul 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&&typeoftemplate==='string'&&StringPrototypeEndsWith(template,'X')){

Uh oh!

fs: make mkdtemp accept buffers and URL#48828

fs: make mkdtemp accept buffers and URL #48828

Uh oh!

Conversation

LiviaMedeiros commented Jul 18, 2023

Uh oh!

LiviaMedeirosJul 18, 2023

Choose a reason for hiding this comment

Uh oh!

aduh95Jul 19, 2023

Choose a reason for hiding this comment

Uh oh!

LiviaMedeirosJul 19, 2023

Choose a reason for hiding this comment

Uh oh!

aduh95Jul 19, 2023

Choose a reason for hiding this comment

Uh oh!

LiviaMedeirosJul 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

fs: make `mkdtemp` accept buffers and URL #48828