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

Stringify parsed schema #266

Closed
magicmatatjahu opened this issue Mar 29, 2021 · 15 comments · Fixed by #402
Closed

Stringify parsed schema #266

magicmatatjahu opened this issue Mar 29, 2021 · 15 comments · Fixed by #402
Labels
enhancement New feature or request

Comments

@magicmatatjahu
Copy link
Member

magicmatatjahu commented Mar 29, 2021

Currently if you want stringify parsed schema you must do it manually by JSON.stringify function. However, here we have a problem with circular schemas and we must add as third parameter custom function to handle these circular schemas.

I suggest to have static stringify function inside AsyncAPIDocument to handle described problem:

class AsyncAPIDocument {
  static stringify(spec) {
    // operation on spec._json to handle circular schemas
    // all internal extensions `x-parser` will be included inside stringified schema
  }
}

Some use cases for this feature:

I can make this logic in mentioned repos but we can add it as feature to parser.

@jonaslagoni
Copy link
Member

I like the idea, might want to add it to Base since one might want to stringify different kind of objects and not just the root.

@derberg
Copy link
Member

derberg commented Mar 29, 2021

do I get it right, you want to parse it to get a string and then pass it to the component that parses it again? or am I missing something? I case I'm right, isn't it better to make react component accept AsyncAPIDocument and skip parsing in the component?

@magicmatatjahu
Copy link
Member Author

magicmatatjahu commented Mar 29, 2021

@derberg You're right about React component, it should accepts the AsyncAPIDocument instance, but problem is with circular schemas inside spec. As I wrote if you parse spec in server-side, you must stringify it before send, but if you have circulars schemas then you will have error with stackoverflow, because JSON.stringify cannot handle deep circular references and our stringify function will be only wrapper for JSON.stringify to handle these circular refs. If something is incomprehensible, let me know :)

Some code how it will look in sever-side and front-end apps:

// server-side
const spec = ...
const parsedSpec = new Parser(...).parse(spec);
const stringifiedSpec = AsyncAPIDocument.stringify(parsedSpec);
response.send(stringifiedSpec, ...)

// front-end app
const parsedSchema = new AsyncAPIDocument(responseSpec);
... render React component with `schema={parsedSchema}` prop

of course this is only one of use cases :)

@derberg
Copy link
Member

derberg commented Mar 29, 2021

yup, but we do not have a use case for now as afaik we do not want the studio to have a server anymore but be client-side.

also the problem with stringify is -> what will be the output? how will those circular refs be presented in stringify spec so they are not lost? did you think about this part?

@magicmatatjahu
Copy link
Member Author

magicmatatjahu commented Mar 29, 2021

yup, but we do not have a use case for now as afaik we do not want the studio to have a server anymore but be client-side.

Studio will still have server-side part for login etc parts. If you don't trust me, go to source code. Studio isn't a Playground, it has a front-end part but still also backend-end part, not for render spec but for other things.

Also I added two use cases in issue description, for Studio and for HTML-Template :)

also the problem with stringify is -> what will be the output? how will those circular refs be presented in stringify spec so they are not lost? did you think about this part?

I know about this part 😄 Reference (object) for circular refs will be lost, but we will still have information that schema is circular by x-parser-circular and x-parser-circular-props extensions. For example you have schema:

A = {
  type: "object",
  properties: {
    normalProp: {
      type: "string",
    },
    circularProp: A,
  }
}

then after parsing and stringification you should have this:

{
  type: "object",
  properties: {
    normalProp: {
      type: "string",
    },
    circularProp: {
      'x-parser-circular': true
    }
  }
  'x-parser-schema-id': 'A',
  'x-parser-circular-props': ['circularProp']
}

We don't have this yet, but we should also have information about inferred schema like:

{
  type: "object",
  properties: {
    normalProp: {
      type: "string",
    },
    circularProp: {
      'x-parser-circular': true,
      'x-parser-inferred-schema': 'A', // schema id, in this case A
    }
  },
  'x-parser-schema-id': 'A'
  'x-parser-circular-props': ['circularProp']
}

@derberg What do you think?

@derberg
Copy link
Member

derberg commented Mar 29, 2021

Studio will still have server-side part for login etc parts. If you don't trust me, go to source code. Studio isn't a Playground, it has a front-end part but still also backend-end part, not for render spec but for other things.

I clicked on the link, saw some .js files but still my opinion did not change :trollface:
I'm not talking about what Studio is now (even though as you noticed, there is no parsing in the server anyway), but what studio is supposed to be later. We want to try to get rid of the server to not pay for hosting like it is done now with the parser, this is why we have the investment in react component that will replace HTML template so no server-side render is needed. So there is definitely no use case to do any parsing on the server cause why would we do that.

Also I added two use cases in issue description, for Studio and for HTML-Template :)

Yes but the HTML template use case we discussed can be solved in a better way by improvement in the react component.
I'm not strongly against the idea, I just don't see a use case that is worth adding such functionality to the template.
Side question, why can you use originalAsyncAPI in the HTML template, I mean pass originalAsyncAPI instead of asyncapi to the component?

@magicmatatjahu
Copy link
Member Author

magicmatatjahu commented Mar 29, 2021

Yes but the HTML template use case we discussed can be solved in a better way by improvement in the react component.

But in which way? 😄 We can make improvements by passing AsyncAPIDocument as a prop, but you need parsed schema (I mean merged traits, made dereference etc) to create the correct instance of the AsyncAPIDocument class, so here is a next problem:

Side question, why can you use originalAsyncAPI in the HTML template, I mean pass originalAsyncAPI instead of asyncapi to the component?

I use the originalAsyncAPI value, because PR is only a draft and I don't want to have stackoverflow for circular schemas (main problem in created issue 😄) - I had it for dummy.yaml from generator. Also, I cannot use the asyncapi value, because this is a reference to object, not string value. Also if I go with asyncapi I need to stringify it, so here is a use case for stringify function.

However, I want two things in HTML-Template:

  • render pure html in template rendering to have SEO, and then hydrate component to attach event handlers when user visit page and for that I need parsed schema or parse it every time when page will be loaded, but:
  • currently html-template has good performance and it is a light template so I don't want to lose these two things. I created this issue (to stringify parsed schema and pass it to React component) and this to leave these two things, I mean not include in final HTML-Template code the whole parser to parse and validate schema, since it has already been parsed 😄

Also for studio issue, do you know that raml content type doesn't work on web, and you need nodejs app to parse your spec? I don't know exactly what the plans are for the Studio, but looking at other ideas like marketplace for specs/extensions/bindings etc server will be needed and for displaying properly parsed spec by component we will probably use two ways: parsing in browser for simple spec and for complex spec by consuming parsed spec from server.

@magicmatatjahu
Copy link
Member Author

Additionally, I need parsed schema for hydration in case when someone pass in spec reference to file.

@derberg
Copy link
Member

derberg commented Mar 30, 2021

tbh I was not aware of how hydration in React works, I'm surprised that in the end there are some things running in the browser anyway. If this is the only way to make it work, instead of the web component then let's do it. Just check to Gatsby to be 100% sure there is no other way

@magicmatatjahu
Copy link
Member Author

I will :)

@magicmatatjahu
Copy link
Member Author

magicmatatjahu commented Apr 13, 2021

@derberg I tested Gatsby and unfornatelly it doesn't work. Gatsby includes whole component with parser and also "stringify" in fly the parsed spec (to include it to the final rendered site) so circular reference in parsed spec throws stackoverflow error. Additionally Gatsby adds an internal code only used by Gatsby so site increases the final size.

Can I then contribute and write the functionality for stringify function? 😄 Will the godfather of the project allow me to do so?

@derberg
Copy link
Member

derberg commented Apr 14, 2021

Will the godfather of the project allow me to do so?

🤦🏼

I understand this is the ONLY solution that can help us out to use react component inside HTML template, so yeah, go ahead. Consider what @jonaslagoni said, that might be useful to have it on each model

@github-actions
Copy link

This issue has been automatically marked as stale because it has not had recent activity 😴
It will be closed in 60 days if no further activity occurs. To unstale this issue, add a comment with detailed explanation.
Thank you for your contributions ❤️

@github-actions
Copy link

This issue has been automatically marked as stale because it has not had recent activity 😴
It will be closed in 60 days if no further activity occurs. To unstale this issue, add a comment with detailed explanation.
Thank you for your contributions ❤️

@magicmatatjahu
Copy link
Member Author

Still valid. It's also connected to the #293 and #294 issues.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants