Skip to content

lib: add defaultValue and name options to AsyncLocalStorage #57766

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

Closed

Conversation

jasnell
Copy link
Member

@jasnell jasnell commented Apr 5, 2025

The upcoming AsyncContext specification adds a default value and name to async storage variables. This adds the same to AsyncLocalStorage to promote closer alignment with the pending spec.

const als = new AsyncLocalStorage({
  name: 'foo',
  defaultValue: 123,
});

console.log(als.name);  // 'foo'
console.log(als.getStore());  // 123

Refs: https://github.com/tc39/proposal-async-context/blob/master/spec.html

@nodejs-github-bot nodejs-github-bot added async_local_storage AsyncLocalStorage needs-ci PRs that need a full CI run. labels Apr 5, 2025
@mcollina
Copy link
Member

mcollina commented Apr 5, 2025

I don't think we should do this right now because the spec is still Stage 2, while AsyncLocalStorage is stable. Those fields could easily be scrapped/changed, and we would be stuck with them.

This comment was marked as outdated.

@jasnell jasnell force-pushed the jasnell/asynclocalstorage-options branch from 0612c1f to 4cf070f Compare April 6, 2025 03:34
@jasnell
Copy link
Member Author

jasnell commented Apr 6, 2025

@mcollina

I don't think we should do this right now because the spec is still Stage 2, while AsyncLocalStorage is stable. Those fields could easily be scrapped/changed, and we would be stuck with them.

I wouldn't have opened the PR if I didn't think they were useful independently of the AsyncContext proposal. The defaultValue in particular is really quite useful and is something we can keep even if it is removed from AsyncContext. While the spec is what prompted me to want to add these, I don't think it should be a blocker to adding them.

Specifically, if we were prematurely implementing AsyncContext itself I would agree with the concern.

@jasnell jasnell added the semver-minor PRs that contain new features and should be released in the next minor version. label Apr 6, 2025
@jasnell jasnell force-pushed the jasnell/asynclocalstorage-options branch from 4ef2590 to 3e68ac6 Compare April 6, 2025 04:15
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

The upcoming `AsyncContext` specification adds a default value and name
to async storage variables. This adds the same to `AsyncLocalStorage`
to promote closer alignment with the pending spec.

```js
const als = new AsyncLocalStorage({
  name: 'foo',
  defaultValue: 123,
});

console.log(als.name);  // 'foo'
console.log(als.getStore());  // 123
```

Refs: https://github.com/tc39/proposal-async-context/blob/master/spec.html
@jasnell jasnell force-pushed the jasnell/asynclocalstorage-options branch from 3e68ac6 to 69a25e2 Compare April 6, 2025 16:22
@jasnell jasnell added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 6, 2025
@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Apr 6, 2025

Co-authored-by: Chengzhong Wu <legendecas@gmail.com>
jasnell added a commit that referenced this pull request Apr 7, 2025
The upcoming `AsyncContext` specification adds a default value and name
to async storage variables. This adds the same to `AsyncLocalStorage`
to promote closer alignment with the pending spec.

```js
const als = new AsyncLocalStorage({
  name: 'foo',
  defaultValue: 123,
});

console.log(als.name);  // 'foo'
console.log(als.getStore());  // 123
```

Refs: https://github.com/tc39/proposal-async-context/blob/master/spec.html
PR-URL: #57766
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
@jasnell
Copy link
Member Author

jasnell commented Apr 7, 2025

Landed in a369818

@jasnell jasnell closed this Apr 7, 2025
JonasBa pushed a commit to JonasBa/node that referenced this pull request Apr 11, 2025
The upcoming `AsyncContext` specification adds a default value and name
to async storage variables. This adds the same to `AsyncLocalStorage`
to promote closer alignment with the pending spec.

```js
const als = new AsyncLocalStorage({
  name: 'foo',
  defaultValue: 123,
});

console.log(als.name);  // 'foo'
console.log(als.getStore());  // 123
```

Refs: https://github.com/tc39/proposal-async-context/blob/master/spec.html
PR-URL: nodejs#57766
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
RafaelGSS pushed a commit that referenced this pull request May 1, 2025
The upcoming `AsyncContext` specification adds a default value and name
to async storage variables. This adds the same to `AsyncLocalStorage`
to promote closer alignment with the pending spec.

```js
const als = new AsyncLocalStorage({
  name: 'foo',
  defaultValue: 123,
});

console.log(als.name);  // 'foo'
console.log(als.getStore());  // 123
```

Refs: https://github.com/tc39/proposal-async-context/blob/master/spec.html
PR-URL: #57766
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
RafaelGSS pushed a commit that referenced this pull request May 2, 2025
The upcoming `AsyncContext` specification adds a default value and name
to async storage variables. This adds the same to `AsyncLocalStorage`
to promote closer alignment with the pending spec.

```js
const als = new AsyncLocalStorage({
  name: 'foo',
  defaultValue: 123,
});

console.log(als.name);  // 'foo'
console.log(als.getStore());  // 123
```

Refs: https://github.com/tc39/proposal-async-context/blob/master/spec.html
PR-URL: #57766
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
@aduh95 aduh95 added the backport-requested-v22.x PRs awaiting manual backport to the v22.x-staging branch. label May 6, 2025
@aduh95
Copy link
Contributor

aduh95 commented May 6, 2025

Tests are not passing on v22.x-staging, this would require a manual backport if we want it on 22.x

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
async_local_storage AsyncLocalStorage author ready PRs that have at least one approval, no pending requests for changes, and a CI started. backport-requested-v22.x PRs awaiting manual backport to the v22.x-staging branch. needs-ci PRs that need a full CI run. semver-minor PRs that contain new features and should be released in the next minor version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants