Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

feat!: v1 #1

Merged
merged 19 commits into from
May 16, 2022
Merged

feat!: v1 #1

merged 19 commits into from
May 16, 2022

Conversation

metcoder95
Copy link
Owner

@metcoder95 metcoder95 commented Apr 18, 2022

Pending

  • Testing
  • Documentation

@metcoder95 metcoder95 marked this pull request as ready for review May 10, 2022 15:15
index.js Outdated

module.exports = fp(
function fastifyRacePlugin (fastify, globalOpts, next) {
const controllers = new Map()

Choose a reason for hiding this comment

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

This looks very leak-prone. Maybe use a WeakMap instead?

Copy link
Owner Author

Choose a reason for hiding this comment

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

I was thinking about that at the first iteration, but I wasn't sure under which circumstances the onResponse cannot be called so the Map is not cleaned properly. But I think for avoiding unexpected issues would be better to go with a WeakMap as you suggested

Copy link
Owner Author

Choose a reason for hiding this comment

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

Btw, here is what do you recommend to use the key for the WeakMap? My initial thought is to take advantage of the request, as is ready to be gc'ed once the request is done. wdyt?

@metcoder95
Copy link
Owner Author

Hi @mcollina please have a look, I refactored the implementation to use WeakMaps instead, thanks for the feedback 🙂

@metcoder95
Copy link
Owner Author

Merging now, please feel free to provide more feedback afterwards 🙂

@metcoder95 metcoder95 merged commit 39f342e into main May 16, 2022
@metcoder95 metcoder95 deleted the feat/v1 branch May 16, 2022 15:53
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants