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

Issue #5320 - using jetty-websocket-httpclient.xml within webapp Jetty 10 #5563

Merged
merged 12 commits into from
Nov 11, 2020

Conversation

lachlan-roberts
Copy link
Contributor

Issue #5320

Make similar changes from PR #5374 for Jetty 10.

  • re-enable the distribution tests for websocket client within webapp.
  • fix the XmlHttpClientProvider so that it uses the HttpClient's classloader
  • add the websocket-jetty-client.jar to jetty-home/lib/websocket/. This is so it can be optionally exposed to the webapp if you wish to use it as a provided dependency.

Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
…ests

Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
@lachlan-roberts lachlan-roberts requested review from gregw and sbordet and removed request for gregw November 4, 2020 00:00
Copy link
Contributor

@sbordet sbordet left a comment

Choose a reason for hiding this comment

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

I don't think jetty-home or POM dependencies should depend on websocket-jetty-client.
If you added those dependencies just for testing, then we should find another way to test without adding the dependencies to the server modules.

Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
@@ -0,0 +1,24 @@
# DO NOT EDIT - See: https://www.eclipse.org/jetty/documentation/current/startup-modules.html

[description]
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove these 2 lines, the link is now obsolete with the new Jetty 10 docs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These lines are present on every single .mod file I can find. Maybe the change to fix all these should all be done in a separate PR?

Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
@lachlan-roberts lachlan-roberts requested a review from gregw November 9, 2020 13:58
Copy link
Contributor

@gregw gregw left a comment

Choose a reason for hiding this comment

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

better... but ....

Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
@lachlan-roberts lachlan-roberts requested a review from gregw November 9, 2020 14:44
Copy link
Contributor

@sbordet sbordet left a comment

Choose a reason for hiding this comment

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

LGTM but please fix the javadocs I refer to in the comment.

@lachlan-roberts lachlan-roberts merged commit 859cf6c into jetty-10.0.x Nov 11, 2020
@lachlan-roberts lachlan-roberts deleted the jetty-10.0.x-5320-WebSocketHttpClient2 branch November 11, 2020 23:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants