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.

Sign up for GitHub

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

Jump to bottom

Expose rhea types #36

Merged
merged 2 commits into from
Apr 10, 2019
Merged

Expose rhea types #36

merged 2 commits into from
Apr 10, 2019

Conversation

ramya0820
Copy link
Contributor

@ramya0820 ramya0820 commented Apr 8, 2019 •
edited
Loading

Description

Brief description of the changes made in the PR.

  • These changes delink library users from depending on Rhea for inferring the underlying types

Reference to any github issues

ramya-rao-a
ramya-rao-a reviewed Apr 8, 2019
View reviewed changes
Copy link
Collaborator

@ramya-rao-a ramya-rao-a left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The first export in this file is from rhea. Can we not add Typed into that list instead of exporting it separately?

amarzavery
amarzavery reviewed Apr 8, 2019
View reviewed changes
lib/index.ts Outdated
@@ -22,3 +22,4 @@ export {
Func, AmqpResponseStatusCode, isAmqpError, ConnectionStringParseOptions, delay, messageHeader,
messageProperties, parseConnectionString, ParsedOutput
} from "./util/utils";
export { Typed } from "rhea/typings/types";
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather than exporting Typed from the file path for types, it would be better to export Typed from rhea.

export { types as Types, Typed } from "./types";

over here and then re-export it in this index.ts file over here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it - submitted PR for this at - amqp/rhea#213

This was referenced Apr 8, 2019
Merged
Closed
@ramya0820
Copy link
Contributor Author

@amarzavery
Looking to merge, publish this PR. Should the version be bumped, or can we update the current one?
Since we are only exposing an additional Type, the changes seem non-intrusive, backward compatible to me.

@amarzavery
Copy link
Collaborator

npm does not allow overwriting an already published version. Please update the patch version of the package for the PR to be merged. Post that @ramya-rao-a can help you publish the package.

@amarzavery
Copy link
Collaborator

Please update the changelog as well.

@amarzavery amarzavery merged commit a014d24 into amqp:master Apr 10, 2019
@ramya0820
Copy link
Contributor Author

Thanks for the merge @amarzavery
Discussed offline with @ramya-rao-a
We'll be publishing the package once few other PRs from team that are in flight are ready.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ramya-rao-a ramya-rao-a ramya-rao-a approved these changes

@amarzavery amarzavery amarzavery approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ramya0820 @amarzavery @ramya-rao-a