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 property descriptor object to calculate hash of getters and setters #121

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

piranna
Copy link

@piranna piranna commented Jan 13, 2023

This fix #120, and also fix a missing validSha1 field, update dist files, and add package-lock file. Only drawback is that change the returned hash since it now takes in account the full property descriptor object, but also helps by generating different hashes when a data and property accessor generates the same value.

@addaleax
Copy link
Collaborator

I think I would consider this surprising behavior – I don’t think I would ever expect that users want to include the function source code backing a getter inside the hash value.

@piranna
Copy link
Author

piranna commented Jan 13, 2023

why not? a getter is not a value, but a computed value... Honetly I did this PR to fix the issue at #120, but on the other hand, this also fix the case where

{a: 1}

and

{get a(){return 1}}

would generate the same hash, while they are clearly NOT the same object nor have the same shape...

@addaleax
Copy link
Collaborator

would generate the same hash

Yeah, that’s expected behavior. I think saying that they are “clearly” not the same shape is very subjective here, and I’m sure that if I accepted this change as it is, there would be numerous complaints about the fact that now these two objects are considered different.

As you can see, this library takes a number of options to configure how exactly it interprets JS objects. I could see a PR that puts a different behavior behind a flag as acceptable, but even then I’m not sure that you really really want to hash a getter’s source code here. You probably rather want to avoid hashing the prototype altogether (it’s not clear from #120 if that’s the input here or not), in which case you may want to just use respectType: false as an existing option here.

@piranna
Copy link
Author

piranna commented Jan 13, 2023

At the other issue, the problem was that It was going into the class object prototype (NOT a class instance) and found the getter, that since It was not an initialized object It failed. In that case, the getter IS the actual value, so i'm not interested un the returned value. Besides that, I can understand that in some cases a getter and a value can be seen the same, but for me It would not be the defsult value. In fact, my original plan for this PR was to fully remove the direct property access and instead use always the property descripción, but somewhere It lead me to some cyclic graph and infinite loop that would need to be manager elsewhere somewhay, for example with a WeakMap.

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.

Not properly hashing class based objects getters
2 participants