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

Use vanilla AsyncLocalStorage instead of asynchronous-local-storage #158

Merged
merged 3 commits into from
Jun 12, 2023

Conversation

voxpelli
Copy link
Contributor

Fixes #148 by removing asynchronous-local-storage in favor of built in AsyncLocalStorage

Requires that Node.js v14 is dropped.

Checklist

index.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
@kibertoad
Copy link
Member

@Fdawgs How can we remove Node 14 from CI if standard workflow is used?

@voxpelli
Copy link
Contributor Author

If some inspiration is needed, then this is how I make the version range possible to overide in my personal standard workflows: https://github.com/voxpelli/ghatemplates/blob/c740bdbac71aec173a791c0dc15b41f0bfca72ef/.github/workflows/test.yml#L11-L15

index.js Outdated Show resolved Hide resolved
@kibertoad
Copy link
Member

kibertoad commented Jun 11, 2023

there are conflicts after last PR was merged

Fixes fastify#148

Signed-off-by: Pelle Wessman <pelle@kodfabrik.se>
@voxpelli
Copy link
Contributor Author

@kibertoad Conflict fix + feedback responded to / fixed.

@kibertoad
Copy link
Member

@voxpelli It seems to work fine on Node 14 too, btw. Should we add engine entry to package.json instead? afair, it needs > specific minor version.

@voxpelli
Copy link
Contributor Author

@kibertoad Its marked as experimental in Node 14, but since Node 14 is no longer being updated I guess that experimental status is now kind of no different to it being stable, as the experimental status only meant that it could receive breaking changes in minor/patch releases?

So I guess we could just merge and ship as is? Without specifying anything new?

An engine definition in the package.json is always nice though

@voxpelli
Copy link
Contributor Author

let's add a test which validates that we are not leaking mutated defaultValues across requests then

I will add tomorrow 👍

@kibertoad
Copy link
Member

@voxpelli Now I wonder if recreating defaultValues on every request is even necessary with how ALS works. can you write a test that would be failing unless we are creating fresh copy every time?

@voxpelli
Copy link
Contributor Author

Sure, I think #136 already added tests to ensure that for the factory, I'll borrow them and verify it

@mcollina
Copy link
Member

It seems that tests are still passing on Node v14.

@kibertoad
Copy link
Member

Actually, yes, Node 14 is good: https://github.com/kibertoad/asynchronous-local-storage/blob/master/lib/nodeVersionUtils.ts

so we only need missing tests and we are good

@voxpelli
Copy link
Contributor Author

@kibertoad Tests been added and verified ✔️

@kibertoad
Copy link
Member

@voxpelli thanks! do tests fail if we don't use fresh copy each time?

@voxpelli
Copy link
Contributor Author

voxpelli commented Jun 12, 2023

@kibertoad Yes the tests fail then. Though this is not an issue in the current code as the current code creates a new Map for every request: https://github.com/kibertoad/asynchronous-local-storage/blob/4504e910c5c88f8f52f74fad046ed7fa2bc8cf63/lib/als.ts#L18

@kibertoad
Copy link
Member

@voxpelli Thanks a lot! Are you planning any more changes, or can I release next semver major?

@voxpelli
Copy link
Contributor Author

voxpelli commented Jun 12, 2023

@voxpelli Thanks a lot! Are you planning any more changes, or can I release next semver major?

@kibertoad No more changes planned! 🙏

@kibertoad kibertoad merged commit 2fab6b5 into fastify:master Jun 12, 2023
14 checks passed
@voxpelli voxpelli deleted the voxpelli/issue148 branch June 12, 2023 10:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ALS is deprecated and archived, Better to use AsyncLocalStorage class fom the async_hook std module
4 participants