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

Values are memoised across class/prototype instances #74

Closed
notjosh opened this issue Apr 12, 2021 · 5 comments · Fixed by #75
Closed

Values are memoised across class/prototype instances #74

notjosh opened this issue Apr 12, 2021 · 5 comments · Fixed by #75
Labels

Comments

@notjosh
Copy link

notjosh commented Apr 12, 2021

Hi!

On the surface, it's kind of logical that (by default) values are instances are memoised across instances, because they're just function calls.

However: a) that seems almost never like intended behaviour, and b) there's a test that seems like it's explicitly trying to test for this.

Unfortunately, the test is only correct as a side effect. If you pass a new initial value into the constructor, you can break the logic (I've fixed the test on my branch, but not sure how to fix the actual problem!)

Do you know how, either as a library consumer, or within the library itself we could get this working as expected?

@fregante
Copy link
Collaborator

Thanks for trying the new decorator and opening an issue. Are you sure that test runs at all? ++this.index sets it to undefined now I think

@notjosh
Copy link
Author

notjosh commented Apr 13, 2021

The tests run ok, but I think you're referring to not defining index on the class? It's using a shorthand syntax for assigning properties directly from the constructor, but is otherwise the same as before. (ref: https://kendaleiv.com/typescript-constructor-assignment-public-and-private-keywords/)

@sindresorhus
Copy link
Owner

// @Richienb

@Richienb
Copy link
Contributor

Richienb commented Apr 13, 2021

Should mem be memoizing values returned by a function across instances or for each instance?

@sindresorhus
Copy link
Owner

For each instance by default at least. We could consider adding an option to opt into memorization across instances.

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 a pull request may close this issue.

4 participants