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

util: add getCWDURL internal util fn #48434

Merged
merged 9 commits into from
Sep 22, 2023

Conversation

jlenon7
Copy link
Member

@jlenon7 jlenon7 commented Jun 12, 2023

Refs: #46826 (comment)

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/loaders
  • @nodejs/modules

@nodejs-github-bot nodejs-github-bot added esm Issues and PRs related to the ECMAScript Modules implementation. needs-ci PRs that need a full CI run. process Issues and PRs related to the process subsystem. util Issues and PRs related to the built-in util module. labels Jun 12, 2023
Copy link
Contributor

@JakobJingleheimer JakobJingleheimer left a comment

Choose a reason for hiding this comment

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

This should probably be applied to all usage of cwd within internal/modules. I think there are 2 others where it's applicable:

const parentPath = parent?.filename ?? process.cwd() + path.sep;

parentURL = pathToFileURL(`${process.cwd()}/`).href;

Copy link
Contributor

@JakobJingleheimer JakobJingleheimer left a comment

Choose a reason for hiding this comment

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

🙌

Copy link
Contributor

@JakobJingleheimer JakobJingleheimer left a comment

Choose a reason for hiding this comment

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

In case CI needs me to re-approve after your push (sometimes it does, sometimes it doesn't 🤪)

Comment on lines 359 to 370
/**
* Get the current working directory while accounting for the possibility that it has been deleted.
* `process.cwd()` can fail if the parent directory is deleted while the process runs.
* @returns {string} The current working directory or the volume root if it cannot be determined.
*/
function getCwdSafe() {
const { sep } = require('path');
Copy link
Member

Choose a reason for hiding this comment

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

shouldn't this be cached at module level, since users can change it?

Suggested change
/**
* Get the current working directory while accounting for the possibility that it has been deleted.
* `process.cwd()` can fail if the parent directory is deleted while the process runs.
* @returns {string} The current working directory or the volume root if it cannot be determined.
*/
function getCwdSafe() {
const { sep } = require('path');
const { sep } = require('path');
/**
* Get the current working directory while accounting for the possibility that it has been deleted.
* `process.cwd()` can fail if the parent directory is deleted while the process runs.
* @returns {string} The current working directory or the volume root if it cannot be determined.
*/
function getCwdSafe() {

Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a legitimate reason to change this and expect it to continue to work?

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with Jordan: it's called getCwdSafe, let's make it safe.

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay sure. But what is the use-case of a user changing sep and expecting things to work? Wouldn't that break heaps of stuff in node?

Copy link
Member

Choose a reason for hiding this comment

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

I think the word “safe” in the function name causes this friction. May I recommend removing the word safe from it?

Copy link
Contributor

Choose a reason for hiding this comment

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

There's no use case I'm aware of to do it, but the platform shouldn't be breakable by user action.

That sounds out of scope then: even if we do this here, node will have exploded long before reaching this. If we need to make sep safe, I think that should be handled all together (in a different PR).

Copy link
Member

Choose a reason for hiding this comment

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

i mean sure, but requiring path at module level seems like the default and idiomatic thing to do; requiring things in function bodies is usually only done when using a module-level caching mechanism. why require it inline here?

Copy link
Contributor

@aduh95 aduh95 Jun 12, 2023

Choose a reason for hiding this comment

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

Let's imagine a user has this snippet somewhere in their code:

Object.defineProperty(require("node:path"), "sep", { __proto__:null, get() { throw this } });

It makes getCwdSafe throw, when the whole point of the function is to never throw.

node will have exploded long before reaching this

Assuming this is true (I don't think it is), we should fix the other places rather than taking it as a reason for not making internals more robust 😅

Copy link
Contributor

Choose a reason for hiding this comment

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

why require it inline here?

Ack; when are we supposed to require it JIT? I thought the point of Joyee's huge PR was to reduce startup overhead (which is incurred by root-level requires).

when the whole point of the function is to never throw

Ah, okay. I do see your point: if that get() → throw is triggered outside, it isn't triggered by the "safe" util.

Copy link
Contributor

@aduh95 aduh95 Jun 12, 2023

Choose a reason for hiding this comment

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

I thought the point of Joyee's huge PR was to reduce startup overhead (which is incurred by root-level requires)

That's true for non-snapshoted modules, node:path is part of the snapshot so the overhead is the same at the top-level as it is inlined. My understanding is that lazy-loading modules already in the snapshot is slightly worth than no lazy-loading, but that's beside the point: we can make this function actually safe, so we should do it IMO :)

The snapshot creation is explained in

// By default, Node.js binaries come with an embedded V8 startup snapshot
// that is generated at build-time with a `node_mksnapshot` executable.
// The snapshot generation code can be found in `SnapshotBuilder::Generate()`
// from `src/node_snapshotable.cc`.
// This snapshot captures the V8 heap initialized by scripts under
// `lib/internal/bootstrap/`, including this file. When initializing the main
// thread, Node.js deserializes the heap from the snapshot, instead of actually
// running this script and others in `lib/internal/bootstrap/`. To disable this
// behavior, pass `--no-node-snapshot` when starting the process so that
// Node.js actually runs this script to initialize the heap.

We have a test to assert that no new modules are added to the bootstrap sequence, and you can see that node:path is already there:

'NativeModule path',

when are we supposed to require it

I'd say at the top-level, like Jordan suggested.

@anonrig
Copy link
Member

anonrig commented Jun 12, 2023

In case CI needs me to re-approve after your push (sometimes it does, sometimes it doesn't 🤪)

@JakobJingleheimer I think this issue still exists. Ref: nodejs/node-core-utils#677

Comment on lines 364 to 385
function getCwdSafe() {
const { sep } = require('path');
let cwd = '';

try {
cwd = process.cwd();
} catch {
/**/
}

return cwd + sep;
}
Copy link
Member

@GeoffreyBooth GeoffreyBooth Jun 12, 2023

Choose a reason for hiding this comment

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

As discussed in the previous PR, let’s change this to always return the same as process.cwd() (so no trailing slash) and update all the call sites to expect as such.

With regard to path.sep, I don’t think we need to use it? Using / as the fallback should work everywhere, including Windows, I would think? Then that avoids the prototype issue.

Suggested change
function getCwdSafe() {
const { sep } = require('path');
let cwd = '';
try {
cwd = process.cwd();
} catch {
/**/
}
return cwd + sep;
}
function getCwdSafe() {
try {
return process.cwd();
} catch {
return '/';
}
}

Or does this need the trailing slash because if the fallback is /, then the code using this needs to assume a trailing slash because when the fallback is returned it would have a trailing slash?

Copy link
Contributor

@aduh95 aduh95 Jun 12, 2023

Choose a reason for hiding this comment

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

I disagree, that's going to create path that look like C:\Users/ or // instead of C:\Users\ and /. It's going to make this change breaking for no good reason.

Also it's not a prototype issue that we are discussing above.

Copy link
Member Author

Choose a reason for hiding this comment

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

Agreed, if we need the trailing slash at the end for some reason, we should do something like ${getCwdSafe()}/.

Copy link
Member

Choose a reason for hiding this comment

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

I disagree

With using / instead of path.sep, or with this not returning a trailing slash?

I’m fine with using path.sep if you have a way of doing so safely, which I’m sure you do. As for the trailing slash, if it’s required because of the fallback, then we should call this function getCwdWithTrailingSlash or getCwdWithTrailingSlashSafe to make it clear.

Copy link
Member Author

@jlenon7 jlenon7 Jun 12, 2023

Choose a reason for hiding this comment

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

Bunch of tests are failing locally with the above implementation, @aduh95 is right.

I have experienced some troubles in the old PR when using process.cwd() without the trailing slash at the end, but we are talking about a safe process.cwd(), and not safe process.cwd()/.

All of my changes do need the trailing slash at the end, but we will certainly have scenarios where we will not need it, shouldn't we remove it and add only where it's really necessary?

Or maybe we should add an argument to the function?

function getCwdSafe(endTrailingSlash = false) {
  let cwd = '';

  try {
    cwd = process.cwd();

    if (endTrailingSlash) {
      cwd += sep;
    }

    return cwd;
  } catch {
    return sep;
  }
}

EDIT: Or do something like @GeoffreyBooth said, getCwdWithTrailingSlashSafe().

Copy link
Member

@GeoffreyBooth GeoffreyBooth Jun 12, 2023

Choose a reason for hiding this comment

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

Maybe we want a getSafeCwdURL util, as I think that’s where the trailing slash is very important?

All but one of the call sites want a URL. Maybe this could be getSafeCwdURL like this?

Suggested change
function getCwdSafe() {
const { sep } = require('path');
let cwd = '';
try {
cwd = process.cwd();
} catch {
/**/
}
return cwd + sep;
}
function getSafeCwdURL() {
try {
return pathToFileURL(process.cwd());
} catch {
return new URL('file:///');
}
}

And the one call site that wants a path could just have the old logic inlined.

Copy link
Member

Choose a reason for hiding this comment

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

@GeoffreyBooth If the result of getSafeCwdURL is not mutated, we can memorize the new URL('file:///') creation for better performance.

Copy link
Member

Choose a reason for hiding this comment

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

If the result of getSafeCwdURL is not mutated, we can memorize the new URL('file:///') creation for better performance.

The fallback is used very, very rarely: only if the cwd has been deleted since the Node app started. So it may not be worth the memory used to save it.

The value of pathToFileURL(process.cwd()), on the other hand, would be worth memorizing assuming that process.cwd cannot change after the app starts? Either as an instantiated URL object or as a string URL. If this gets called often enough, it might be worth creating something like process.cwdFileURL that returns the cwd as a string URL directly from C++?

Copy link
Member

Choose a reason for hiding this comment

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

Looks like a really good optimization @GeoffreyBooth.

Copy link
Contributor

Choose a reason for hiding this comment

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

file:/// is not a valid file: URL for Windows unfortunately, we'd have to use pathToFileURL('/') to be portable unfortunately.

Comment on lines 7 to 10
 at Module._compile (node:internal*modules*cjs*loader:1256:14)
 at Module._extensions..js (node:internal*modules*cjs*loader:1310:10)
 at Module.load (node:internal*modules*cjs*loader:1114:32)
 at Module._load (node:internal*modules*cjs*loader:961:12)
Copy link
Member

Choose a reason for hiding this comment

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

@MoLow shouldn’t these line/column numbers be obscured so that unrelated changes don’t require updates to snapshots?

Copy link
Member

Choose a reason for hiding this comment

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

I agree. Maybe we should replace them with *

@GeoffreyBooth
Copy link
Member

Something we should discuss is whether the fallback behavior in the original getCwdSafe is a good idea. For example, look at the call site in the CommonJS loader:

    const parentPath = parent?.filename ?? getCwdSafe();
    const pkg = readPackageScope(parentPath) || {};

What readPackageScope does with this input is search for a package.json file in the current folder, then in its parent, then onward up the tree until it either reaches the root or is told that it’s not allowed to proceed any higher by a permissions check. So the behavior that his PR adds to the CommonJS loader here is a tiny security hole: by deleting the CWD after the app begins, that permissions check is bypassed and Node tries to read /package.json from the volume root. (Though if an application can go so far as delete the folder from which the application was launched, presumably they already have permissions to read a file from the root.)

What if instead we cached the CWD on application startup and just always returned that? Then there would be no need for the fallback or the try/catch; the string or URL instance could always be returned. The call site would need to decide how or if it wanted to handle the possibility that the path had been deleted since server startup; maybe it would decide to use the root of the volume instead, but presumably some of the time it could just return a default value (like the readPackageScope case, where it could return whatever would be returned if no package.json files were found even when searching up to the root) or it could error. But that should probably be a case-by-case decision rather than something abstracted in a helper.

@ljharb
Copy link
Member

ljharb commented Jun 12, 2023

Would process.chdir still alter the cached value?

@jlenon7
Copy link
Member Author

jlenon7 commented Jun 12, 2023

My goal when creating getSafeCwd() was to minimize the code, removing try/catch blocks that make things a bit difficult to read and also reuse code by abstracting it in a helper.

I agree that it should be a case-by-case decision, but we have to pay attention to the fact that I'm not using getSafeCwd() in all places (trying to replace process.cwd() everywhere), the goal was to use it only where we got the
try/catch blocks.

So maybe I should remove it from:

const parentPath = parent?.filename ?? getCwdSafe();

And:

parentURL = pathToFileURL(getCwdSafe()).href;

Also, IMO, renaming it to something like getSafeCwdURL() and return an instance of a URL is a very nice approach, since we are always using pathToFileURL(getSafeCwd()).href everywhere.

@GeoffreyBooth
Copy link
Member

I’m not using getSafeCwd() in all places (trying to replace process.cwd() everywhere), the goal was to use it only where we got the
try/catch blocks.

The one in cjs/loader.js isn’t using a try/catch, so I’d revert that change. It’s also the only one expecting a path rather than a URL, so it could be left for another PR.

Since all the others expect a URL, you could make the helper function return a URL. Looking a little closer, all three of the remaining call sites are using the helper to define parentURL; and file:/// isn’t actually correct (that’s not a module’s parent’s URL) so arguably returning / or the Windows equivalent isn’t really what we should be doing for our fallback. I think arguably the most correct behavior in these cases is probably to return what the CWD was, since I think these are all referencing the main entry point of the application and at the time, the CWD existed. So we could create a helper like this:

const cwdAtStartup = pathToFileURL(process.cwd());

function getCwdAtStartup() {
  return cwdAtStartup;
}

And I don’t think we need a try around this because this code would run on startup before the application logic had a chance to delete the CWD. And I would add the trailing slash at the calling sites rather than include it the return value of the helper, for consistency with process.cwd.

So then the question becomes, does anything break if we define a parentURL to be a path that doesn’t exist? And if so, I guess that’s where we would need to put try/catch blocks, and in those cases the catch should be something more specific to the context of that part of the code (return a default value, error, etc.).

What do others think?

@aduh95
Copy link
Contributor

aduh95 commented Jun 13, 2023

And I don’t think we need a try around this because this code would run on startup before the application logic had a chance to delete the CWD.

The CWD can be deleted by something that's not the application, in all likelihood node is not the only process running. It's a flawed reasoning to only take node into account, it's still a race condition. I'm not saying we should try/catch it, probably it's fine if it fails at this point, but it would be wrong to assume it can't throw.

Note that process.cwd() already returns a cached value (well only if node owns its own state), see:

// Cache the working directory to prevent lots of lookups. If the working
// directory is changed by `chdir`, it'll be updated.
let cachedCwd = '';

function wrappedCwd() {
if (cachedCwd === '')
cachedCwd = rawMethods.cwd();
return cachedCwd;
}

@aduh95
Copy link
Contributor

aduh95 commented Jun 13, 2023

I think we need to create a util that looks like

let cachedURL;
let cachedCWD;
function getCWDURL() {
  let cwd;
  try {
    cwd = process.cwd() + sep;
  } catch {}
  if (cwd != null && cwd !== cachedCWD) {
    cachedURL = pathToFileURL(cwd);
    cachedCWD = cwd;
  }
  return cachedURL;
}

I think that would take care of all the edge cases, while avoid re-creating URL instances. wdyt?

@GeoffreyBooth
Copy link
Member

Note that process.cwd() already returns a cached value

If that’s the case, why/when would it ever throw? It tries to validate the path’s existence even though it’s been cached? So another solution could be process.cwd({ validate: false }) to disable the check and just return the cached value?

@aduh95
Copy link
Contributor

aduh95 commented Jun 14, 2023

Note that process.cwd() already returns a cached value

If that’s the case, why/when would it ever throw? It tries to validate the path’s existence even though it’s been cached? So another solution could be process.cwd({ validate: false }) to disable the check and just return the cached value?

No it always return the cached value when there's one, there's no validation, look at the code I shared above. There's no cached value when node doesn't own its own state (that's a build flag).


@GeoffreyBooth
Copy link
Member

No it always return the cached value when there’s one

I’m asking about process.cwd, not your proposed new function. If process.cwd returns a cached value, what are the circumstances in which it would throw? Leaving aside this alternate build mode that Node could be built as; assuming that the “does own process state” mode is the default, when can process.cwd throw? The tests involve deleting the CWD path before calling process.cwd(), but why would that cause an exception if a cached value was returned?

@aduh95
Copy link
Contributor

aduh95 commented Jun 15, 2023

I'm not sure what to do to make myself clear, I'm soon going to run out of code snippets to share 😅

Wether if node is build with kOwnsProcessState build flag (which is included with kDefaultFlags):

node/src/node.h

Lines 614 to 619 in 718f62b

// Use the default behaviour for Node.js instances.
kDefaultFlags = 1 << 0,
// Controls whether this Environment is allowed to affect per-process state
// (e.g. cwd, process title, uid, etc.).
// This is set when using kDefaultFlags.
kOwnsProcessState = 1 << 1,

it's going to affect which file between internal/bootstrap/switches/does_own_process_stat or internal/bootstrap/switches/does_not_own_process_stat gets included in the binary

node/src/node_realm.cc

Lines 339 to 342 in ac0853c

auto process_state_switch_id =
env_->owns_process_state()
? "internal/bootstrap/switches/does_own_process_state"
: "internal/bootstrap/switches/does_not_own_process_state";

In the default case (i.e. all the official binaries), node own its own state, meaning it can meaningfully cache the value of process.cwd() (that's the wrappedCwd function, if you run node -p process.cwd.name you should see wrappedCwd) and it can't throw except at the first invocation.

In the case where node is built by an embedder (i.e. Electron and the likes) that has turned off the built flag, node doesn't own its own state, so we can't cache and the raw libuv function (the one that can throw) is used every time.

TL;DR process.cwd() is already as optimised as it can be, but still it can throw in some rare edge cases, and I think the implementation I suggested in #48434 (comment) is our best shot.

I'd like to point out that I'm not an expert in process.cwd, I got all the above information from reading the code, which I encourage you to do, because while I agree it's a good thing to brainstorm optimisation ideas, the conversation would be a lot more productive if I wasn't the only one checking if they actually make sense 🙈 I have the feeling that you make me work extra time to explain to you our codebase, and I think it's not fair, you should either accept my conclusion and trust it to be good, or do the actual work of verifying it (FWIW I'd prefer the latter, I'm always happy to be proven wrong). I'm under the impression that my options in this thread is either to keep drafting detailed comments and getting not very informed questions the next day as a response, or to stop participating. Anyway, sorry for my rant, I'll retire from this thread now.

@GeoffreyBooth
Copy link
Member

GeoffreyBooth commented Jun 15, 2023

To me, there are two issues here:

  1. If process.cwd is caching its initial value (for default builds) then I would think that it should never throw. If it still does, that feels like a bug. This can be investigated in a separate PR, if it’s something we feel like dealing with.

  2. The current call sites of getCwdSafe all seem to be able to survive with an invalid/not-very-sensical value for parentURL returned by the current getCwdSafe. I think the best solution of all would be to see if these call sites can be refactored to behave sensibly if parentURL is undefined. Then we don’t need getCwdSafe at all and can get rid of it. Each call site could look like this:

let parentURL;
try {
  parentURL = pathToFileURL(cwd() + sep);
} catch {
  /* Sometimes cwd() throws, in which case parentURL remains undefined */
}

// Following code should be able to handle a possibly undefined parentURL

This can be abstracted if desired, but the same goal would apply: rather than returning the root of the volume as the parent URL when CWD cannot be determined, just return undefined; and the code using that return value should be able to continue working when that return value is undefined. Is such a refactoring possible @jlenon7?

Alternatively, if it’s required that parentURL always be defined (either because we can’t make the code work otherwise or because there’s some spec/UX reason that it should always be defined, like so that we always pass in a defined value to custom loader hooks) then the question becomes what should it be when CWD is unknown. Should it be a cached value of CWD, and if so, why? Should it be the root of the volume, and if so, why? Once we answer that question, we can create an abstracted function like getParentURL to define the logic, based on one of the proposed getCwdSafe versions proposed above.

@jlenon7
Copy link
Member Author

jlenon7 commented Sep 7, 2023

People, sorry for disappearing 😅. I was moving to Lisbon. Now things are much calmer and I can get back with this PR.

I'm going to follow the implementation that @aduh95 mentioned, wdyt?

I think we need to create a util that looks like

let cachedURL;
let cachedCWD;
function getCWDURL() {
  let cwd;
  try {
    cwd = process.cwd() + sep;
  } catch {}
  if (cwd != null && cwd !== cachedCWD) {
    cachedURL = pathToFileURL(cwd);
  }
  return cachedURL;
}

I think that would take care of all the edge cases, while avoid re-creating URL instances. wdyt?

This function was first implemented in nodejs#46826, but at some point
of the PR implementation this fn was no longer related to the PR.

Refs: nodejs#46826 (comment)
Use `getCwdSafe` in other scenarios of `internal/modules` package.
Increment the line number in `.snapshot` file since we have add
the `require()` instruction of `getCwdSafe` in
`internal/modules/cjs/loader.js` file.
Implement a function that can handle a second
cachedCWD when Node.js process doesn't owns it
own state.
@aduh95 aduh95 added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels Sep 21, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 21, 2023
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@aduh95 aduh95 added commit-queue Add this label to land a pull request using GitHub Actions. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. labels Sep 22, 2023
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Sep 22, 2023
@nodejs-github-bot nodejs-github-bot merged commit c2cd744 into nodejs:main Sep 22, 2023
33 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in c2cd744

ruyadorno pushed a commit that referenced this pull request Sep 28, 2023
This function was first implemented in #46826, but at some point
of the PR implementation this fn was no longer related to the PR.

Refs: #46826 (comment)
PR-URL: #48434
Reviewed-By: Jacob Smith <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
This was referenced Sep 28, 2023
alexfernandez pushed a commit to alexfernandez/node that referenced this pull request Nov 1, 2023
This function was first implemented in nodejs#46826, but at some point
of the PR implementation this fn was no longer related to the PR.

Refs: nodejs#46826 (comment)
PR-URL: nodejs#48434
Reviewed-By: Jacob Smith <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
targos pushed a commit to targos/node that referenced this pull request Nov 11, 2023
This function was first implemented in nodejs#46826, but at some point
of the PR implementation this fn was no longer related to the PR.

Refs: nodejs#46826 (comment)
PR-URL: nodejs#48434
Reviewed-By: Jacob Smith <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
targos pushed a commit to targos/node that referenced this pull request Nov 11, 2023
This function was first implemented in nodejs#46826, but at some point
of the PR implementation this fn was no longer related to the PR.

Refs: nodejs#46826 (comment)
PR-URL: nodejs#48434
Reviewed-By: Jacob Smith <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
@targos targos added the backported-to-v18.x PRs backported to the v18.x-staging branch. label Nov 23, 2023
targos pushed a commit that referenced this pull request Nov 23, 2023
This function was first implemented in #46826, but at some point
of the PR implementation this fn was no longer related to the PR.

Refs: #46826 (comment)
PR-URL: #48434
Backport-PR-URL: #50669
Reviewed-By: Jacob Smith <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
sercher added a commit to sercher/graaljs that referenced this pull request Apr 25, 2024
This function was first implemented in #46826, but at some point
of the PR implementation this fn was no longer related to the PR.

Refs: nodejs/node#46826 (comment)
PR-URL: nodejs/node#48434
Backport-PR-URL: nodejs/node#50669
Reviewed-By: Jacob Smith <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
sercher added a commit to sercher/graaljs that referenced this pull request Apr 25, 2024
This function was first implemented in #46826, but at some point
of the PR implementation this fn was no longer related to the PR.

Refs: nodejs/node#46826 (comment)
PR-URL: nodejs/node#48434
Backport-PR-URL: nodejs/node#50669
Reviewed-By: Jacob Smith <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. backported-to-v18.x PRs backported to the v18.x-staging branch. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. esm Issues and PRs related to the ECMAScript Modules implementation. needs-ci PRs that need a full CI run. process Issues and PRs related to the process subsystem. util Issues and PRs related to the built-in util module.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants