search

asyncapi / parser-js

AsyncAPI parser for Javascript (browser-compatible too).
Apache License 2.0
116 stars 92 forks source link

Road to v2.0.0 #481

Closed magicmatatjahu closed 1 year ago

magicmatatjahu commented 2 years ago

This issue contains list of possible features that we wanna introduce in the v2.0.0 of ParserJS. Have in mind that some of them may not be implemented. You can also write your own proposals/features/PRs in the comments, I will update then the main list.

Note: Mainly PRs should be merged to the next-major branch. Additionally, some of the following issues will only be fixable/implementable once the other issues are resolved.

Nice To Have

cc @jonaslagoni @smoya

smoya commented 2 years ago

@magicmatatjahu would you mind adding https://github.com/asyncapi/parser-js/issues/482 into the list?

magicmatatjahu commented 2 years ago

@smoya Added :)

smoya commented 2 years ago

@magicmatatjahu as https://github.com/asyncapi/parser-js/issues/294 is not really a BC, we can remove it from the list so we move forward just with the minimum.

smoya commented 2 years ago

@magicmatatjahu as https://github.com/asyncapi/parser-js/issues/405 is not really a BC, we can remove it from the list so we move forward just with the minimum.

smoya commented 2 years ago

@magicmatatjahu as https://github.com/asyncapi/parser-js/issues/404 is not really a BC, we can remove it from the list so we move forward just with the minimum.

smoya commented 2 years ago

@magicmatatjahu I'm concerned about https://github.com/asyncapi/parser-js/issues/403 as this would become a BC, so perfect for the new parser version, but it requires a lot of effort. Do you think we should invest on this for the next version of the Parser? cc @jonaslagoni @fmvilas

smoya commented 2 years ago

@magicmatatjahu can you annotate https://github.com/asyncapi/parser-js/issues/201 is done?

smoya commented 2 years ago

@magicmatatjahu as https://github.com/asyncapi/parser-js/issues/157 is not really a BC, we can remove it from the list so we move forward just with the minimum.

smoya commented 2 years ago

We could build another list, instead of removing issues, with the Nice To Have issues. In case there is time or people willing to take those.

EDIT: Here is the list @magicmatatjahu . You can add it right after the current list on the description of this issue.

Nice To Have

smoya commented 2 years ago

@magicmatatjahu This is going to be needed as well https://github.com/asyncapi/parser-api/issues/69

smoya commented 2 years ago

After giving another thought to the list of tasks for v2.0.0 of the parser, I think we can go further and reduce the scope. I considered AsyncAPI Roadmap at all moments to see how we could deliver value asap (the new API).

2022-07-26 at 20 47 12@2x

Here is what I would suggest we could move with, but I know @magicmatatjahu had strong reasons not to discard some of the tasks, so I'm going to ask you if you could expand info on why you believe those are needed. I'm addressing to you since you created most of those issues, also because you have a good knowledge of the v1.x code base, however, anyone is invited and encouraged to jump into this discussion so we can find the minimum required features for the new version.

jonaslagoni commented 2 years ago

Tbh, the only thing I see as required would be https://github.com/asyncapi/parser-js/issues/482 (because that's already underway) and https://github.com/asyncapi/parser-js/issues/401 (because well, that is what triggers this rewrite).

My reason for only those two is that is what is directly needed from the generator's perspective and probably where #401 is needed the most.

That said, remember... You can do another major release in another month if that's what you would like to do where you address even more of these issues. Nobody is stopping you from releasing new major versions of the parser.

magicmatatjahu commented 2 years ago

@smoya

https://github.com/asyncapi/parser-js/issues/477 -> Ok, it's a really cool feature from the developer point of view, but could we copy the current parser-js logic into the new TS version?

I already explained you. For me it would be too much pointless work, because from the beginning there was a thought to use Spectral and for that we have collaborations with Spotlight team regarding the official ruleset AsyncAPI. However if community thinks that it's better idea (it's usually right, isn't?) we can do this, but we should discuss that. cc @derberg @jonaslagoni @fmvilas

https://github.com/asyncapi/parser-js/issues/480 -> Is it really needed for this version? If we don't move into spectral atm, I think it's not.

If we do not go with Spectral then it is not needed, but the function validate is made to use it later in the context of Spectral and to return errors when validating the used schema in a custom format, that is, where the problem occurred, etc. Currently, the given solution does not have this, and for me this is a serious problem from the UX level - the user does not know where the error is and sometimes also what problem, because he/she see only runtime error, not meaningful error for human,

https://github.com/asyncapi/parser-js/issues/119 -> we can create a common interface for those errors. Once we integrate spectral, those could be later on be adapted to our own interface so we won't introduce any breaking change.

For this we have in mentorship program that project https://github.com/imabp/asyncapi_problem but with Spectral we won't need that in 99% cases. As I think there, even in the case of using logic from our parser, it should not be necessary. I would throw this issue from the scope and even close it.

smoya commented 2 years ago

I already explained you.

Yeah, but we talked privately so I wanted to add visibility to this, opening the discussion to whoever is interested in this whole community.

because from the beginning there was a thought to use Spectral and for that we have collaborations with Spotlight team regarding the official ruleset AsyncAPI.

I understand moving to Spotlight is something we must eventually do. The fact we unify validation logic into just one place, the way Spectral does validation based on rules, etc, is way better to what we have right now in current parser. However, by reading https://github.com/asyncapi/parser-js/issues/477, and even being aware of the work @jonaslagoni and @magicmatatjahu did in Spectral, I can't see we agreed anywhere to become part of v3.0 as a must.

On that side, I agree @jonaslagoni on the following statement:

You can do another major release in another month if that's what you would like to do where you address even more of these issues. Nobody is stopping you from releasing new major versions of the parser.

Having said that, I would love to know what is the status of the development of AsyncAPI rules on Spectral. Is there any rule that the current parser has but Spectral doesn't? Maybe there is an issue tracking the progress that I'm missing (sorry if that's the case).

I'm asking that because if we are done in Spectral side, maybe we could evaluate moving forward with only the following in addition to what I mentioned:

Thoughts? Ideas?

magicmatatjahu commented 2 years ago

@smoya

Having said that, I would love to know what is the status of the development of AsyncAPI rules on Spectral. Is there any rule that the current parser has but Spectral doesn't? Maybe there is an issue tracking the progress that I'm missing (sorry if that's the case).

https://github.com/stoplightio/spectral/issues/2100

If we have consensus then we can go with the old logic.

smoya commented 2 years ago

stoplightio/spectral#2100

That means only one task is missing to at least match current parser implementation: https://github.com/stoplightio/spectral/issues/1103

If that's correct, it is very close to being done on Spectral side. I wonder how difficult is to integrate Spectral with the new version of the parser + moving forward with https://github.com/asyncapi/parser-js/issues/329.

magicmatatjahu commented 2 years ago

https://github.com/stoplightio/spectral/issues/1103 is not needed at all, because it's very custom rule to check if any component in components section is used or not. We have optimizer for that.

If that's correct, it is very close to being done on Spectral side. I wonder how difficult is to integrate Spectral with the new version of the parser + moving forward with https://github.com/asyncapi/parser-js/issues/329.

You can check code https://github.com/asyncapi/parser-js/blob/next-major/src/parser.ts Parser uses Spectral as you see. Also I don't understand this part about https://github.com/asyncapi/parser-js/issues/329 issue. Spectral handles it for us.

smoya commented 2 years ago

Also I don't understand this part about https://github.com/asyncapi/parser-js/issues/329 issue. Spectral handles it for us.

Gotcha, I see this refers to all custom validations, which are now in Spectral rules. 👍

Anyway, my intention was to remove the two following issues from the roadmap of v2 in case we finally move forward with Spectral:

smoya commented 2 years ago

Btw, I noticed we are never speaking about deadlines or any estimated completion date. I know we usually don't work with such but I think we should consider the following as in favour to set them:

Having said that, IMHO, late September/early October 2022 seems like an ideal moment, leaving at least 6 months of testing/improving before having a v3 release candidate to test with.

We have to decide whether to use Spectral or discard it for this version so we can deliver on time. I'm going to write another comment with the factors to help making such a decision.

WDYT? cc @jonaslagoni @dalelane @derberg @magicmatatjahu @fmvilas @Souvikns and anyone interested!

smoya commented 2 years ago

We have to decide whether to use Spectral or discard it for this version so we can deliver value "asap" and on time (TBD, see the previous comment). Thanks to the latest comments made by @magicmatatjahu, We have now a better perspective of Spectral integration progress.

The summary of Spectral integration development status could be summarized now as:

Work to be done

1. If we move forward with Spectral, the pending work, at this moment will be:

2. If we discard Spectral for this version, the pending work, at this moment will be:

Is there anything I'm missing @magicmatatjahu ?

My opinion, please discuss:

The reality is that if traveling in time existed, I would travel to the past and postpone Spectral integration for another version rather than v2.0.0, focusing the development of such version on the new API. However, at the current stage, I think it's now viable to move forward with Spectral as it is deeply integrated already and the cost to move forward (discarding some tasks) seems to be similar or even lower than to get it back.

I would like to know what are your thoughts on this.

cc @jonaslagoni @dalelane @derberg @magicmatatjahu @fmvilas @Souvikns and anyone interested.

magicmatatjahu commented 2 years ago

As for the removal of Spectral, it is not necessary to reimplement custom parsers, because the API then does not change, the new API (for custom parsers) is needed to make sure that parsers will return errors from Spectral context.

I would describe it more as:

  1. If we move forward with Spectral, the pending work, at this moment will be:

  2. If we discard Spectral for this version, the pending work, at this moment will be:

I already wrote, if the community decides to go without spectral then we go without it. I will adjust and do not take "active" participation in this discussion.

derberg commented 2 years ago

To port custom validations https://github.com/asyncapi/parser-js/blob/master/lib/customValidators.js

I think this is the key in discussion about spectral in or out. Writing custom validators is time-consuming, requires lots of testing and is vulnerable to changes to spec. I know, I wrote them (of course it may also be a case that they could be written better 😄 but don't tell me that, I have feelings 😆 ). This is also why we still do not have all the validations in place (for example message example validation).

I see @magicmatatjahu already extended AsyncAPI Spectral rule with custom validation. Only uniqueness of messageId is missing.

I'm only concerned about errors. Can you share please how validation error will look like when spectral notices that operationId is duplicated? It is not clear to me from your conversation, so I need to see a real example before I can share my comment on that topic.

Independently from above, what is the problem really 😆

My position is: When we plan 1.0 of some library, it makes sense to set some list of stuff that we fill are needed to be there for the first production release. But 2.0, 3.0, 4.0 - in only blocks development are release of improvements really 😄 and forces work on feature branches, and doing multiple features at the same time and blocking each other 😄

For example, if you want spectral, and you started a discussion a long time ago in https://github.com/asyncapi/parser-js/issues/477, and there were no objections => then just propose how you wanna do it, and do it 😆

Spectral has nothing to do with TypeScript and nothing to do with new parser API. Why can't we jump from parser 1.0 to 4.0 in a year? It is not a harm to the community, but actually a good thing. People do not care about numbers as they have package-lock to protect them. They care about improvements.

magicmatatjahu commented 2 years ago

@derberg

I'm only concerned about errors. Can you share please how validation error will look like when spectral notices that operationId is duplicated? It is not clear to me from your conversation, so I need to see a real example before I can share my comment on that topic.

There are tests for that, you can check (and also for other rules in other files) outputs: https://github.com/stoplightio/spectral/blob/develop/packages/rulesets/src/asyncapi/__tests__/asyncapi-operation-operationId-uniqueness.test.ts

We can always change description of error.

derberg commented 2 years ago

So from what I see, spectral can be introduced separately, probably even as not a breaking change, we just transform their errors to ours, because we use json pointer to point to location where error is. We also do not need severity for now, as we anyway want to integrate errors only first, right?

anyway, for me fundamentally most important is to drop thinking about road to 2.0, 3.0 etc 😄 as parser and other libraries evolve separately from the spec.

fmvilas commented 2 years ago

My two cents: go with what @derberg is saying. You don't need to decide what goes into v2.0.0. Just move ahead with whatever you already have. Don't make things depend on each other when they're not dependent, really. Want Spectral? Cool, add it. Want Typescript? Cool, add it too. But that should not block a new API. These are 3 different things. And it's ok to end up with Parser 6.0.0 in, say, November.

magicmatatjahu commented 2 years ago

@derberg

So from what I see, spectral can be introduced separately, probably even as not a breaking change, we just transform their errors to ours, because we use json pointer to point to location where error is. We also do not need severity for now, as we anyway want to integrate errors only first, right?

I would not share this opinion, because if we want to integrate Spectral we should go with its output from beginning and not convert it to our output (I am talking about the result of validation). The strength of Spectral is that you can create spec governance of the company/organization with it and these severity in this regard are super important.

If we weren't to go with Spectral now, then 2.0.0 we go only with the new API, and in 3.0.0 the improved API after testing with Spectral integration.

derberg commented 2 years ago

@magicmatatjahu I added a follow-up comment about spectral in https://github.com/asyncapi/parser-js/issues/477#issuecomment-1216430740 to not "pollute" this thread with technical details of just one feature.

If we weren't to go with Spectral now, then 2.0.0 we go only with the new API, and in 3.0.0 the improved API after testing with Spectral integration.

maybe 🤷🏼 tbh I do not know how complicated it is to integrate Spectral nor how advanced work on the new API is 😄 maybe we get Spectral in sooner 😄 More important is that we agree that we do not need major release planning and roadmaps

magicmatatjahu commented 2 years ago

Leftover tasks:

Required tasks in other repos:

Review/Discussion:

Nice to have:

cc @smoya @derberg @fmvilas

smoya commented 2 years ago

@magicmatatjahu you can mark Model updates (fixes) according to parser-api API. as done as it has been merged

smoya commented 2 years ago

@magicmatatjahu can you mark Traversing as done as it has been merged? Also for the record leave the link to the merged PR: https://github.com/asyncapi/parser-js/pull/604. Thanks!

smoya commented 1 year ago

@magicmatatjahu could u please add https://github.com/asyncapi/parser-js/issues/620 to the list of TODO?

smoya commented 1 year ago

Also this one @magicmatatjahu https://github.com/asyncapi/parser-api/issues/69

magicmatatjahu commented 1 year ago

@smoya Done.

fmvilas commented 1 year ago

What's really stopping us from releasing Parser v2? I want to move this forward as soon as possible so we can start receiving feedback.

smoya commented 1 year ago

What's really stopping us from releasing Parser v2? I want to move this forward as soon as possible so we can start receiving feedback.

There is missing work that should go in Spec v3.0.0 that still is not supported in the Parser. Champions are willing to work on it. See https://github.com/asyncapi/spec/issues/691#issue-1108390273

Some kind of summary: