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

Search by class name instead of the class itself #64

Closed
wants to merge 4 commits into from
Closed

Search by class name instead of the class itself #64

wants to merge 4 commits into from

Conversation

gagdiez
Copy link
Contributor

@gagdiez gagdiez commented Jul 14, 2023

Right now we are getting lots of errors when trying to deserialise the exact same class across different versions of near-api-js, near-social-vm and others, since the SCHEMA indexes by the Class itself.

Therefore, I have changed the search inside the schema, so we look for the Class name instead of the Class. In this way, different versions of Transaction will point to the same schema.

This is a non breaking change that solves the problem without changing the Schemas, nor the expected behaviour of the serialised.

@gagdiez gagdiez requested a review from volovyks July 14, 2023 14:18
@gagdiez gagdiez requested a review from ailisp as a code owner July 14, 2023 14:18
@gagdiez gagdiez changed the title Index by class name instead of the class itself Search by class name instead of the class itself Jul 14, 2023
Copy link

@MaximusHaximus MaximusHaximus left a comment

Choose a reason for hiding this comment

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

Thanks for the PR -- this has been on the radar for a while and it will be great to handle these types of cases.

Just a small syntax suggestion that can avoid mutating a local variable by using more idiomatic JS -- and 1 question about why we changed how the Schema type is being exported; otherwise I think it would be great to get this landed.

I tend to lean towards this being a breaking change/major release, though, since it fundamentally changes the behaviour of the library; previously there was an implicit guarantee that any serialization done by Borsh would absolutely 100% of the time serialize the exact class instances provided in the schema, and error if any other class was provided even if it happened to have the same constructor name. With this change, no error would be thrown and Borsh will make a best-effort to serialize using any constructor that happens to have the same name as the constructor in the schema, even if it might be a totally different class that might serialize data in an entirely different way.

lib/index.js Outdated Show resolved Hide resolved
lib/index.d.ts Outdated Show resolved Hide resolved
borsh-ts/index.ts Outdated Show resolved Hide resolved
gagdiez and others added 3 commits July 14, 2023 17:03

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
Co-authored-by: Daryl Collins <daryl@darylcollins.com>

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
Co-authored-by: Daryl Collins <daryl@darylcollins.com>
@gagdiez
Copy link
Contributor Author

gagdiez commented Jul 14, 2023

@MaximusHaximus while I agree that, in practice, it is a breaking change (since there was an implicit guarantee), I do not see such guarantee as a fundamental aspect of the library for the library users... I personally see it more as a bug.

Specially since the error on (de)serialisation should be dictated by the schema itself, more than the specific Class or Class name. If you pass a valid schema, then Borsh should serialise it, if the schema is invalid, then an error should raise. In my (completely biased) view, one should be able to do serialize(some schema, {object without class}).

Moreover, I do not know (nor expect to be honest to find) anyone to have wanted (or relied on) the specific feature of "index schemas by exact Class", while I do know of many multiple instances in which the behaviour was more a detriment or a bug.

Because of this, I would expect a "minor" or "fix" release to create more well than harm (actually, I would expect it to create 0 problems). Of course my view is biased, and I might be very wrong.

@ailisp
Copy link
Member

ailisp commented Jul 18, 2023

I think this is a breaking change too. While it may not lead to breaking behaviors for most use cases, include different version of libraries shipped with same schema of Transaction, it could lead to breaking behaviors for certain users. (Could a user intentionally rely on this detriment feature to handle different versions of a struct?)

Making it a breaking change will explicitly ask users to examine their use case, and for most users it should be no additional work.

@gagdiez
Copy link
Contributor Author

gagdiez commented Jul 21, 2023

closing in favor of #65

@gagdiez gagdiez closed this Jul 21, 2023
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.

None yet

3 participants