-
-
Notifications
You must be signed in to change notification settings - Fork 98
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
feat: add multiparser to the parserjs monorepo repository. #1036
feat: add multiparser to the parserjs monorepo repository. #1036
Conversation
feat: export types so any other libraries can use them
* update dependencies and add support for v3
packages/multi-parser/package.json
Outdated
"@asyncapi/protobuf-schema-parser": "^3.0.0", | ||
"@asyncapi/raml-dt-schema-parser": "^4.0.4", | ||
"parserapiv1": "npm:@asyncapi/parser@^2.1.0", | ||
"parserapiv2": "npm:@asyncapi/parser@3.0.0-next-major-spec.8" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where is the parserapiv3
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes this is shifted to line number 65. as "@asyncapi/parser": "file:../parser"
which will take the latest version as a @asyncapi/parser
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But why devDependency?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually I was thinking as the parser is in the local now will not need to keep it as dependency. but now i am thinking though the test are passing successfully it should be in dependency only. anyway updated the added it back.
@smoya please can you merge this. |
LOL i hope it is not an order 😂. Will review and merge when i have a chance. I'm out on holidays, and sick at the same time. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First review round
turbo.json
Outdated
"build": {} | ||
"test": {"cache": false}, | ||
"prepublishOnly": {"cache": false}, | ||
"test:unit": {"cache": false}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpick: Does this file have a wrong indentation now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ping
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added the correct indentation.
packages/multi-parser/package.json
Outdated
"@asyncapi/raml-dt-schema-parser": "^4.0.4", | ||
"parserapiv1": "npm:@asyncapi/parser@^2.1.0", | ||
"parserapiv2": "npm:@asyncapi/parser@3.0.0-next-major-spec.8", | ||
"@asyncapi/parser": "file:../parser" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any reason for changing the dependency name? If there isnt, please do not modify the original code from https://github.com/smoya/multi-parser-js/blob/19750c914386d4417c9e17b13584634132805e27/package.json#L47
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Previously, we used parserv3, but now that the repositories are merged and multiparser is using the latest version, it makes sense to refer to it by the correct name. If anyone needs to know the version, they can simply check the version of parser.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason the version was explicit is exactly because we wanted it to be explicit for each version
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ping @ayushnau
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok fixed this.
How would new releases of the |
---
"@asyncapi/parser": minor
"@asyncapi/multi-parser": patch
changes. yes that would mean we dont need to do anything else to sync the parser with multiparser it will be automatically synced |
I guess it works by now. We could work on some automation in the future for this. 👍 |
will be giving update on this by on weekend. currently cant take it. |
@smoya updated this please check. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! 🚀🌔
/rtm |
🎉 This PR is included in version 3.2.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
Changes Added.
Related issue(s)
Related to: #963