fastify / fastify-request-context

Request-scoped storage support, based on Asynchronous Local Storage (with fallback to cls-hooked)
MIT License
154 stars 15 forks source link

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

Closed voxpelli closed 1 year ago

voxpelli commented 1 year ago

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

Requires that Node.js v14 is dropped.

Checklist

kibertoad commented 1 year ago

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

voxpelli commented 1 year ago

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

kibertoad commented 1 year ago

there are conflicts after last PR was merged

voxpelli commented 1 year ago

@kibertoad Conflict fix + feedback responded to / fixed.

kibertoad commented 1 year ago

@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 commented 1 year ago

@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 commented 1 year ago

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

I will add tomorrow 👍

kibertoad commented 1 year ago

@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 commented 1 year ago

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

mcollina commented 1 year ago

It seems that tests are still passing on Node v14.

kibertoad commented 1 year ago

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 commented 1 year ago

@kibertoad Tests been added and verified ✔️

kibertoad commented 1 year ago

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

voxpelli commented 1 year ago

@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 commented 1 year ago

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

voxpelli commented 1 year ago

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

@kibertoad No more changes planned! 🙏