Skip to content

Separate MessagePackHubProtocol into its own package #25253

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

Merged
merged 18 commits into from
Sep 2, 2020

Conversation

@ghost ghost added the area-signalr Includes: SignalR clients and servers label Aug 25, 2020
@BrennanConroy
Copy link
Member

Don't you need a separate .javaproj and dependencies file etc.?

@wtgodbe
Copy link
Member Author

wtgodbe commented Aug 26, 2020

@BrennanConroy @halter73 is the goal to get two separate .jars, or two separate packages within the signalr .jar we already produce? Still trying to figure out how to do the second, if that's what we need (this does the first)

@wtgodbe
Copy link
Member Author

wtgodbe commented Aug 26, 2020

Don't you need a separate .javaproj and dependencies file etc.?

The only .javaproj in the folder is signalr.client.java.Tests.javaproj, for the tests. I had assumed we defined the signalr.jar build in build.gradle

@BrennanConroy
Copy link
Member

signalr.client.java.Tests.javaproj, for the tests

That's a small hack, it's actually for tests and the package.

the goal to get two separate .jars

This.

@wtgodbe wtgodbe added this to the 5.0.0-rc1 milestone Aug 28, 2020
@wtgodbe wtgodbe force-pushed the wtgodbe/MessagePackage branch from 10f4772 to 3de4db5 Compare August 31, 2020 17:19
@wtgodbe wtgodbe force-pushed the wtgodbe/MessagePackage branch from 3de4db5 to b9244b8 Compare August 31, 2020 17:22
@wtgodbe wtgodbe marked this pull request as ready for review August 31, 2020 17:22
@wtgodbe
Copy link
Member Author

wtgodbe commented Aug 31, 2020

@wtgodbe
Copy link
Member Author

wtgodbe commented Aug 31, 2020

@Pilchie asking for this to go in for rc1 - it separates MessagePack into its own package, matching what we do in the C# & TypeScript clients. Also allows us to remove unneeded dependencies from each package

@wtgodbe wtgodbe added ask-mode This issue / PR is a patch candidate which we will bar-check internally before patching it. Servicing-consider Shiproom approval is required for the issue labels Aug 31, 2020
@ghost
Copy link

ghost commented Aug 31, 2020

Hello human! Please make sure you've included the Shiproom Template in a comment or (preferably) the PR description. Also, make sure this PR is not marked as a draft and is ready-to-merge.

@sebastienros
Copy link
Member

@wtgodbe haven't looked at it at all yet

Copy link
Member

@BrennanConroy BrennanConroy left a comment

Choose a reason for hiding this comment

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

We can figure out the VSCode test stuff later

@Pilchie
Copy link
Member

Pilchie commented Sep 1, 2020

Well, I'd rather get this in RC1 than later, so...

Approved for RC1 pending CI completion if merged before 10am Pacific on 2020-09-01.

@Pilchie Pilchie added Servicing-approved Shiproom has approved the issue and removed Servicing-consider Shiproom approval is required for the issue labels Sep 1, 2020
dependencies {
implementation project(':core')
compile 'org.msgpack:msgpack-core:0.8.20'
compile 'org.msgpack:jackson-dataformat-msgpack:0.8.20'
Copy link
Member

Choose a reason for hiding this comment

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

When I try using compile in build.gradle it warns "compile is deprecated; replace with implementation"

Copy link
Member Author

Choose a reason for hiding this comment

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

What command did you run to produce that warning? I don't see it locally w/ compileJava or createPackage

Copy link
Member

Choose a reason for hiding this comment

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

I saw this in Android Studio

image

@Pilchie Pilchie added Servicing-consider Shiproom approval is required for the issue and removed Servicing-approved Shiproom has approved the issue labels Sep 1, 2020
@ghost
Copy link

ghost commented Sep 1, 2020

Hello human! Please make sure you've included the Shiproom Template in a comment or (preferably) the PR description. Also, make sure this PR is not marked as a draft and is ready-to-merge.

@Pilchie
Copy link
Member

Pilchie commented Sep 1, 2020

Removed approval since the deadline passed. Please update the shiproom template for it.

@jamshedd jamshedd added Servicing-approved Shiproom has approved the issue and removed Servicing-consider Shiproom approval is required for the issue labels Sep 1, 2020
@jamshedd
Copy link
Member

jamshedd commented Sep 1, 2020

Approved for RC1.

1 similar comment
@jamshedd
Copy link
Member

jamshedd commented Sep 1, 2020

Approved for RC1.

@wtgodbe
Copy link
Member Author

wtgodbe commented Sep 1, 2020

Follow-up issues: #25510 #25511 #25512

@wtgodbe wtgodbe merged commit b44c628 into release/5.0 Sep 2, 2020
@wtgodbe wtgodbe deleted the wtgodbe/MessagePackage branch September 2, 2020 00:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-signalr Includes: SignalR clients and servers ask-mode This issue / PR is a patch candidate which we will bar-check internally before patching it. Servicing-approved Shiproom has approved the issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants