Fix: maps, sets and vector types #203

osalkanovic · 2022-09-02T14:20:40Z

Issue: #199

…ector-types

ailisp · 2022-09-07T02:18:54Z

src/collections/lookup-map.ts

@@ -13,7 +13,7 @@ export class LookupMap {
        return near.storageHasKey(storageKey)
    }

-    get(key: Bytes): unknown | null {
+    get(key: Bytes): T | null {


The issue is what this get is an object instead of T. For example, the CarSpec get will be {id, color, price, engine}, not CarSpec{id, color, price, engine}

ailisp · 2022-09-07T02:23:36Z

src/collections/unordered-map.ts

    let indexRaw = getIndexRaw(this.keyIndexPrefix, key);
    if (indexRaw) {
      let index = deserializeIndex(indexRaw);
      let value = this.values.get(index);
      if (value) {
-        return value;
+        return value as T;


as T also doesn't solve the problem, it's only a ts sytax suger. When inspect the built JS contract, it is ignored. For example: https://www.typescriptlang.org/play?#code/LAKFGMBsEMGdYAQGFoCcDKAHApuBBvUBYhcAewDtYAXVAV3GrNQApM6AjSASzwugC22AFwIaqbhQDmASgIBfUIrAhI2aggAeCALwF+Q0QHJoR5Wo0BPXVoRxkaLLgDcESrDJqAdJDJSW1pI00BTg2GQAZg4YOOAyQA

@ailisp This could be solved by the new brand feature of ES2022 (here). Using TS syntax here is better for DX since the developer won't look at the compiled code anyway.

Looks like this feature is not related to deserialization. Can you please elaborate on how it can help?

When constructing a collection we could set the private field (brand) to the type of the underlying data we store, this would be serialized and stored which means we could theoretically infer the type with this field when deserializing the collection. We could also just let the user decide what the type is which is easier, and I don't know how valuable inferring the type is anyway. That's why I am saying it doesn't really matter that it says as T since types in JS/TS are just for developer experience anyway

I'm not sure that this solution here is 100% okay, but something in this direction could go:

https://www.typescriptlang.org/play?#code/LAKFGMBsEMGdYAQGFoCcDKAHApuBBvUBYhcAewDtYAXVAV3GrNQApM6AjSASzwugC22AFwIaqbhQDmASgIBfUIrAqocRAFUKzACbZU2HQFlomADwAVAHwEiJclVoMmrdl14ID0HZUgBPBEkaaApwEQQLOUIQEgRlO2IpbGoWGVELBAAfBAo6SEhbGNiSSGSEAA8EAF4CfiFRAHJoBuVikgB6doABalgAWm4pbQME4oNqOlQKHOwAdwRqAAtuWAA6IOoQsJZymVHY0eV4kFLqBAFTapn5rV19QxNMFhQMHHAZAG4IShoEAJqLphVkkUnsQA5YGRSqtIGQpCwAhsttgyAAzZBoLC4AA0fwA-Ks6tgwUA

I don't know if it's okay to ask the user to pass us an instance of the class to constructor :D

@petarvujovic98 what are you think about this example?

@osalkanovic I wouldn't use this approach as the instance is going to also get serialized so you will lose the constructor, plus if it is a primitive type it won't need to have a constructor and you are complicating the DX.

Keeping track of type data after seralization is difficult in JS, but since users are using TS to write the construction and access of variables after deserialization, it's better to keep the logic for those in the TS layer rather than in the JS/runtime layers.

I agree with @petarvujovic98 , and the way @osalkanovic shown is quite clever that I didn't think of. May @osalkanovic 's way be incorporated in as a pattern to write deserializer?: (value: unknown) => T

@ailisp I will update PR with deserializer today.

@ailisp @volovyks @petarvujovic98 PR is ready for review

ailisp

Thanks for the PR! There's still some problems on recovering the type of the object, so still cannot consider it as T 🤔

src/collections/unordered-map.ts

src/collections/unordered-set.ts

src/collections/vector.ts

osalkanovic · 2022-09-08T12:58:42Z

src/collections/unordered-map.ts

@@ -59,21 +59,21 @@ export class UnorderedMap {
    return keysIsEmpty;
  }

-  get(key: Bytes): unknown | null {
+  get(key: Bytes, deserializer?: (value: unknown) => DataType): DataType | null {


@petarvujovic98 since I see that you are working on the default value, maybe it would be good to convert this to options

@osalkanovic Seems reasonable to me. This also allows for more extensibility in the future

ailisp

Overall your work looks great! Thanks!

I'd like to add a small naming comment: in near-bindgen.ts, we call the process from object to DataType as "reconstruct", and deserializer in libraries usually mean from string/bytes to object. So I wonder if we should name it as reconstructor instead of deserializer?

The other way is make deserializer calls on raw instead of on value, this allow user to even override the JSON.parse with a customizable format, but this seems to be more scoped than this PR intended and a serializer? argument is better to come together.

Therefore I suggest naming it as reconstructor, and make a room for future adding deserializer the get by changing the signatures from:

    get(key: Bytes, reconstructor?: (value: unknown) => DataType): DataType | null {

to

    get(key: Bytes, options?: {reconstructor?: (value: unknown) => DataType}): DataType | null {

And, could you please change a test in tests/src/unordered-map.ts to ensure get with option works. We have an existing test that does manually reconstruct:

near-sdk-js/tests/src/unordered-map.js

Line 61 in ef90663

get_house() {

With your PR a house will be get directly!

osalkanovic · 2022-09-09T18:58:10Z

@ailisp Updated with latest develop, ready for merge! :)

ailisp · 2022-09-13T01:36:18Z

@osalkanovic Fantastic! You have even implemented generics for the new lookup map. I think this PR is in production-ready quality. @volovyks If this also looks good to you, let's wait NEARCON end (at Sep 14 and merge this!)

ailisp · 2022-09-15T09:35:59Z

@osalkanovic We turned on ci, if you could resolve and merge latest develop into PR. If everything is good on CI we can merge it!

osalkanovic · 2022-09-16T00:53:11Z

@osalkanovic We turned on ci, if you could resolve and merge latest develop into PR. If everything is good on CI we can merge it!

I will update today

osalkanovic · 2022-09-16T02:54:54Z

@ailisp Ready for merge

ailisp · 2022-09-16T03:02:05Z

@osalkanovic Thank you!

osalkanovic added 2 commits August 30, 2022 23:42

fix: update typescript for vector, sets and maps

0a56ce3

Merge branch 'develop' of github.com:near/near-sdk-js into fix/maps-v…

f126f2b

…ector-types

osalkanovic requested review from ailisp and volovyks as code owners September 2, 2022 14:20

fix: merge conflicts

ca99f9a

osalkanovic marked this pull request as draft September 2, 2022 14:38

osalkanovic added 3 commits September 2, 2022 16:53

fix: prettier

9b70073

fix: code cleaning

116298d

Delete yarn.lock

2845329

osalkanovic marked this pull request as ready for review September 2, 2022 14:58

fix: return yarn.lock from develop

1a449a3

osalkanovic changed the title ~~Fix: maps, sets vector types~~ Fix: maps, sets and vector types Sep 2, 2022

ailisp reviewed Sep 7, 2022

View reviewed changes

petarvujovic98 reviewed Sep 7, 2022

View reviewed changes

src/collections/unordered-map.ts Outdated Show resolved Hide resolved

petarvujovic98 reviewed Sep 7, 2022

View reviewed changes

src/collections/unordered-set.ts Outdated Show resolved Hide resolved

petarvujovic98 reviewed Sep 7, 2022

View reviewed changes

src/collections/vector.ts Outdated Show resolved Hide resolved

petarvujovic98 mentioned this pull request Sep 7, 2022

Collection nesting #202

Closed

osalkanovic added 2 commits September 8, 2022 10:45

fix: fixing typo

a3b24d1

fix: include deserializer in get params

bbbcda1

osalkanovic requested review from ailisp, volovyks and petarvujovic98 and removed request for volovyks, ailisp and petarvujovic98 September 8, 2022 12:57

osalkanovic requested review from volovyks and ailisp and removed request for volovyks September 8, 2022 12:57

osalkanovic commented Sep 8, 2022

View reviewed changes

ailisp approved these changes Sep 9, 2022

View reviewed changes

osalkanovic added 2 commits September 9, 2022 20:34

fix: code cleaning

1492d09

fix: resolving conflicts

332369e

osalkanovic requested a review from ailisp September 9, 2022 18:57

fix: removing logs

35f536f

This was referenced Sep 13, 2022

Refactor the project and add generics and getter options #213

Merged

Unusable nested collections #217

Closed

rename plain object -> class instance operation to "reconstruct" #218

Merged

osalkanovic and others added 4 commits September 16, 2022 03:02

fix: resolve merge conflicts

a5c32d8

fix: code cleaning

966c321

fix: build lib

1db085a

Merge branch 'develop' into fix/maps-vector-types

1496cc6

osalkanovic mentioned this pull request Sep 16, 2022

Mappings and other complex types (hackathon feedback) #222

Closed

ailisp merged commit e8d1288 into near:develop Sep 16, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix: maps, sets and vector types #203

Fix: maps, sets and vector types #203

osalkanovic commented Sep 2, 2022

ailisp Sep 7, 2022

ailisp Sep 7, 2022

petarvujovic98 Sep 7, 2022

volovyks Sep 7, 2022

petarvujovic98 Sep 7, 2022

osalkanovic Sep 7, 2022

osalkanovic Sep 7, 2022

petarvujovic98 Sep 7, 2022

ailisp Sep 8, 2022

osalkanovic Sep 8, 2022

osalkanovic Sep 8, 2022

ailisp left a comment

osalkanovic Sep 8, 2022

petarvujovic98 Sep 8, 2022

ailisp left a comment

osalkanovic commented Sep 9, 2022

ailisp commented Sep 13, 2022

ailisp commented Sep 15, 2022

osalkanovic commented Sep 16, 2022

osalkanovic commented Sep 16, 2022

ailisp commented Sep 16, 2022

Fix: maps, sets and vector types #203

Fix: maps, sets and vector types #203

Conversation

osalkanovic commented Sep 2, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ailisp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ailisp left a comment

Choose a reason for hiding this comment

osalkanovic commented Sep 9, 2022

ailisp commented Sep 13, 2022

ailisp commented Sep 15, 2022

osalkanovic commented Sep 16, 2022

osalkanovic commented Sep 16, 2022

ailisp commented Sep 16, 2022