refactor: add extensions method to the bindings #587

magicmatatjahu · 2022-08-31T17:24:44Z

Description

Add missed extensions method to the Bindings model
Fix types for AsyncAPISchemaObject
Fix types for Collection abstract model
Add unit tests

Related issue(s)
Part of #481
Part of #482

src/models/v2/message.ts

src/models/v2/security-requirement.ts

src/schema-parser/asyncapi-schema-parser.ts

magicmatatjahu · 2022-09-05T15:49:09Z

@smoya Ok, I applied all suggestions. Could you check again?

package-lock.json

smoya · 2022-09-06T08:02:13Z

src/models/base.ts

  json<K extends keyof J>(key: K): J[K];
  json(key?: keyof J) {
-    if (key === undefined) return this._json;
+    if (key === undefined || typeof this._json !== 'object') return this._json;


I don't get why we need this change. what type could be other than object? If the user provides a key, it is expected to only return a value.

The value can also be different from the object - we have one case in our spec, schema can have a boolean, true or false value.

But what if the user provides a value for key? You should still assume the user wants the value for such a key, no matters the type of this._json.

Right, I will fix it.

Done, new function body:

if (key === undefined) return this._json; if (this._json === null || this._json === undefined) return this._json; return this._json[key];

smoya · 2022-09-06T09:17:37Z

src/models/collection.ts

-  abstract has(id: string): boolean;
+  get(id: string): T | undefined {
+    const possibleItem = this._meta.originalData?.[id];
+    return typeof possibleItem === 'undefined' ? this.__get(id) : possibleItem;


Shouldn't this be just the opposite? First we see if __get is declared, if so, means there is a desire from the subclass to override the whole get?

Good point, but have in mind that __get here is treated as fallback to retrieve data when originalData (which can be omitted) doesn't exist or given key also doesn't exist. With your solution we need to duplicate const possibleItem = this._meta.originalData?.[id]; logic in every __get function.

What I suggest is that we do the opposite; something like:

const possibleItem = this.__get(id); return typeof possibleItem === 'undefined' ? this._meta.originalData?.[id] : possibleItem;

But by this you will decrease optimization, I made looking for object first to improve read speed. Also originalData is optional so we can pass objects to the collections constructor only when we will have 100% sure that keys in objects === ids in collections.

Correct me if I'm wrong, please. Imagine having the following document:

components: messages: test1: messageId: 'test' test2: messageId: 'test1'

What .components().messages().get('test1') should return?
I expect messageId should have preference over the key, so I would say the returned message would be:

test2: messageId: 'test1'

However, with your change, that won't happen since this._meta.originalData?.[id] will match with the other message.

I can remove that if you want, because atm we only use it in the extensions case for bindings. WDYT?

I'm concerned about the Developer (of the parser) Experience, not the final user.
I understand the benefit; kind of o(1) VS o(n) lookups, However I'm always in favor of not introducing early optimizations in code, and this one might be one, especially if not executed thinking on DX.

As a last effort to keep it, maybe at least renaming the originalData var to indexedCollection or similar and adding a comment on what this is about could help.

Just to clarify, this could also be used for example for servers and channels (v2) right?

I opt for originalData because in bindings model we need to pass original data (not changed), because then we need to read from original bindings field all extensions for extensions() method. indexedCollection indicates that this data is transformed.

Just to clarify, this could also be used for example for servers and channels (v2) right?

Yeah, also for bindings and extensions models - in every collections where we change object to array 1:1 - we change key to the id of element.

I will remove __get, but we need originalData meta field.

indexedCollection doesn't indicate that data is transformed but just indexed by something (we can assume by id), which can be perfectly the original data.

Yeah, but we can even transform to the object original data - for example messages we can transform to the object, where keys will be messageIds, so we need originalData and probably indexedCollection.

Ok, I removed the __get function. We will think about optimization after release.

smoya · 2022-09-06T09:21:16Z

src/models/extension.ts

@@ -1,7 +1,7 @@
 import type { BaseModel } from "./base";

-export interface ExtensionInterface extends BaseModel {
+export interface ExtensionInterface<T = any> extends BaseModel {


Would T be ever any other type rather than any ?

Extension can include any value, primitive, object, array.

From slack:

You can have any value in extension, we cannot say what exactly type is in extension, but user can cast extension value to given type, so he/she can do this:

const extension = model.extensions().get<TwitterExtensionsShape>('twitter');

and then:

extensions.value()

will return

TwitterExtensionsShape

type.

Yeah. Makes totally sense. Thanks for clarifying!

sonarqubecloud · 2022-09-06T10:17:12Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
No Duplication information

smoya

LGTM! 🚀

magicmatatjahu · 2022-09-06T10:25:24Z

/rtm

magicmatatjahu added the enhancement New feature or request label Aug 31, 2022

magicmatatjahu force-pushed the next/fix-collections-of-bindings branch from 1dbe848 to db1217e Compare September 1, 2022 14:01

magicmatatjahu marked this pull request as ready for review September 1, 2022 14:47

magicmatatjahu requested review from fmvilas, jonaslagoni, derberg and asyncapi-bot-eve as code owners September 1, 2022 14:47

smoya reviewed Sep 2, 2022

View reviewed changes

src/models/v2/message.ts Outdated Show resolved Hide resolved

smoya reviewed Sep 2, 2022

View reviewed changes

src/models/v2/security-requirement.ts Show resolved Hide resolved

smoya reviewed Sep 2, 2022

View reviewed changes

src/schema-parser/asyncapi-schema-parser.ts Outdated Show resolved Hide resolved

magicmatatjahu added 8 commits September 6, 2022 09:25

refactor: add extensions method to the bindings

7d7ea7d

fix type

41d17a6

fix typo

933e6b4

fix types

69ac6f1

fix components model

5fccdf0

fix types

0e010e2

fix tests

3354fa1

after review

b497904

magicmatatjahu force-pushed the next/fix-collections-of-bindings branch from 66581e1 to b497904 Compare September 6, 2022 07:29

smoya reviewed Sep 6, 2022

View reviewed changes

package-lock.json Outdated Show resolved Hide resolved

fix version of package-lock

8722ce1

magicmatatjahu requested a review from smoya September 6, 2022 07:40

smoya reviewed Sep 6, 2022

View reviewed changes

fix json() function

c99d807

magicmatatjahu requested a review from smoya September 6, 2022 08:47

smoya reviewed Sep 6, 2022

View reviewed changes

remove __get

7c16f70

magicmatatjahu requested a review from smoya September 6, 2022 10:20

smoya approved these changes Sep 6, 2022

View reviewed changes

asyncapi-bot added the ready-to-merge label Sep 6, 2022

asyncapi-bot merged commit 3db1d09 into asyncapi:next-major Sep 6, 2022

magicmatatjahu deleted the next/fix-collections-of-bindings branch September 6, 2022 10:25

magicmatatjahu added a commit to magicmatatjahu/parser-js that referenced this pull request Oct 3, 2022

refactor: add extensions method to the bindings (asyncapi#587)

9c2e6e8

derberg pushed a commit that referenced this pull request Oct 4, 2022

refactor: add extensions method to the bindings (#587)

24bd4f9

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: add extensions method to the bindings #587

refactor: add extensions method to the bindings #587

magicmatatjahu commented Aug 31, 2022 •

edited

Loading

magicmatatjahu commented Sep 5, 2022

smoya Sep 6, 2022

magicmatatjahu Sep 6, 2022

smoya Sep 6, 2022

magicmatatjahu Sep 6, 2022

magicmatatjahu Sep 6, 2022

smoya Sep 6, 2022

magicmatatjahu Sep 6, 2022

smoya Sep 6, 2022

magicmatatjahu Sep 6, 2022

smoya Sep 6, 2022 •

edited

Loading

magicmatatjahu Sep 6, 2022

smoya Sep 6, 2022

magicmatatjahu Sep 6, 2022

smoya Sep 6, 2022 •

edited

Loading

magicmatatjahu Sep 6, 2022 •

edited

Loading

smoya Sep 6, 2022

magicmatatjahu Sep 6, 2022

magicmatatjahu Sep 6, 2022

smoya Sep 6, 2022

sonarqubecloud bot commented Sep 6, 2022

smoya left a comment

magicmatatjahu commented Sep 6, 2022

refactor: add extensions method to the bindings #587

refactor: add extensions method to the bindings #587

Conversation

magicmatatjahu commented Aug 31, 2022 • edited Loading

magicmatatjahu commented Sep 5, 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

smoya Sep 6, 2022 • edited Loading

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

smoya Sep 6, 2022 • edited Loading

Choose a reason for hiding this comment

magicmatatjahu Sep 6, 2022 • edited Loading

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

sonarqubecloud bot commented Sep 6, 2022

smoya left a comment

Choose a reason for hiding this comment

magicmatatjahu commented Sep 6, 2022

magicmatatjahu commented Aug 31, 2022 •

edited

Loading

smoya Sep 6, 2022 •

edited

Loading

smoya Sep 6, 2022 •

edited

Loading

magicmatatjahu Sep 6, 2022 •

edited

Loading