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

chore(forestadmin-client): bump lru-cache to latest version #709

Merged
merged 1 commit into from
Jul 4, 2023

Conversation

Thenkei
Copy link
Contributor

@Thenkei Thenkei commented May 31, 2023

Definition of Done

General

  • Write an explicit title for the Pull Request, following Conventional Commits specification
  • Test manually the implemented changes
  • Validate the code quality (indentation, syntax, style, simplicity, readability)

Security

  • Consider the security impact of the changes made

@Thenkei Thenkei changed the title fix(forestadmin-client): bump lru-cache to latest version chore(forestadmin-client): bump lru-cache to latest version [force release] May 31, 2023
`${renderingId}`,
);
)) as RenderingPermission;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do you need this type cast here?

Copy link
Contributor Author

@Thenkei Thenkei May 31, 2023

Choose a reason for hiding this comment

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

Because the fetch method returns Promise<Void | ValueType>

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, but it indicates that the function can return nothing if the value is not present in the cache, which is the case also with this line.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, it is a bit complicated.
In our case, the value is fetched (using the fetch method defined for the LRU) and always available (even if it's undefined).

I don't know how to handle correctly those void that won't happen

Copy link
Contributor

@ghusse ghusse May 31, 2023

Choose a reason for hiding this comment

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

void is undefined.

To be fair, I think that the declaration of this function should be undefined | T instead of void | T.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree. I'm gonna submit a PR on LRU cache.

Copy link

Choose a reason for hiding this comment

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

The issue is a little bit complicated.

It is allowed for the fetchMethod provided to LRUCache to return actually nothing in the event that the fetch is aborted. An abort signal can be fired if the key is deleted or overwritten during the fetch (which can happen under load if the cache evicts old members which are in the process of being fetched), or if the caller explicitly aborts the fetch by providing an AbortSignal of their own.

It's rather annoying (imo) that TS treats void so differently from undefined when referring to a value in this way, since it is literally undefined in that case. It is definitely not type safe to assume that the value will always be defined, though.

I'm open to updating LRUCache to make that function V | undefined, but it's a SemVer major change, which can be a pain. (At minimum, you'd have to update LRUCache.) In the meantime, you can make TS be a bit less snippy about it by doing:

(permissions === undefined ? undefined : permissions)?.collections?.[collectionName];

Copy link
Contributor Author

Choose a reason for hiding this comment

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

isaacs/node-lru-cache#302
isaacs/node-lru-cache#303

The owner of LRU Cache is not willing to make any changes that soon. He proposed a patch we could use it with https://www.npmjs.com/package/patch-package.

Copy link

Choose a reason for hiding this comment

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

Patch-package won't work I don't think, because it's compiled ts. The better workaround is to cast the void to undefined with a conditional assignment to undefined if it's equal to undefined.

@Thenkei Thenkei changed the title chore(forestadmin-client): bump lru-cache to latest version [force release] chore(forestadmin-client): bump lru-cache to latest version May 31, 2023
@Thenkei Thenkei force-pushed the fix/forestadmin-client-interface branch from 891e5f6 to 203f5e6 Compare June 21, 2023 10:56
@Thenkei Thenkei force-pushed the fix/forestadmin-client-interface branch from 203f5e6 to ee37232 Compare June 21, 2023 11:08
Copy link
Contributor Author

@Thenkei Thenkei left a comment

Choose a reason for hiding this comment

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

🚀 Nice addition in lru-cache v10.0.0.

@codeclimate
Copy link

codeclimate bot commented Jun 21, 2023

Code Climate has analyzed commit ee37232 and detected 0 issues on this pull request.

The test coverage on the diff in this pull request is 100.0% (98% is the threshold).

This pull request will bring the total coverage in the repository to 99.1% (0.0% change).

View more on Code Climate.

@Thenkei Thenkei merged commit 3bb7a8e into main Jul 4, 2023
@Thenkei Thenkei deleted the fix/forestadmin-client-interface branch July 4, 2023 09:08
@forest-bot
Copy link
Member

🎉 This PR is included in version 1.0.13 🎉

The release is available on npm package (@latest dist-tag)

Your semantic-release bot 📦🚀

@forest-bot
Copy link
Member

🎉 This PR is included in version 1.3.29 🎉

The release is available on example@1.3.29

Your semantic-release bot 📦🚀

@forest-bot
Copy link
Member

🎉 This PR is included in version 1.13.2 🎉

The release is available on npm package (@latest dist-tag)

Your semantic-release bot 📦🚀

@forest-bot
Copy link
Member

🎉 This PR is included in version 1.10.0 🎉

The release is available on npm package (@latest dist-tag)

Your semantic-release bot 📦🚀

@forest-bot
Copy link
Member

🎉 This PR is included in version 1.0.36 🎉

The release is available on npm package (@latest dist-tag)

Your semantic-release bot 📦🚀

@forest-bot
Copy link
Member

🎉 This PR is included in version 1.3.0 🎉

The release is available on npm package (@latest dist-tag)

Your semantic-release bot 📦🚀

@forest-bot
Copy link
Member

🎉 This PR is included in version 1.0.26 🎉

The release is available on npm package (@latest dist-tag)

Your semantic-release bot 📦🚀

@forest-bot
Copy link
Member

🎉 This PR is included in version 1.5.3 🎉

The release is available on npm package (@latest dist-tag)

Your semantic-release bot 📦🚀

@forest-bot
Copy link
Member

🎉 This PR is included in version 1.4.2 🎉

The release is available on npm package (@latest dist-tag)

Your semantic-release bot 📦🚀

@forest-bot
Copy link
Member

🎉 This PR is included in version 1.3.3 🎉

The release is available on npm package (@latest dist-tag)

Your semantic-release bot 📦🚀

@forest-bot
Copy link
Member

🎉 This PR is included in version 1.7.2 🎉

The release is available on npm package (@latest dist-tag)

Your semantic-release bot 📦🚀

@forest-bot
Copy link
Member

🎉 This PR is included in version 1.5.2 🎉

The release is available on npm package (@latest dist-tag)

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants