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 #9233 - Fix some JPMS issues for websocket-core #9234

Merged

Conversation

lachlan-roberts
Copy link
Contributor

@lachlan-roberts lachlan-roberts commented Jan 31, 2023

Issue #9233

make ServerUpgradeRequest and ServerUpgradeResponse interfaces so we do not expose the WebSocketNegotitation needed to construct them, as the WebSocketNegotitation has now been moved to the internal package.

@joakime joakime linked an issue Jan 31, 2023 that may be closed by this pull request
@joakime joakime changed the title fix some jpms issues for websocket-core Issue #9233 - Fix some JPMS issues for websocket-core Jan 31, 2023
@joakime joakime added this to the 12.0.x milestone Jan 31, 2023
@gregw
Copy link
Contributor

gregw commented Feb 1, 2023

I have some open source concern with the way that we have created internal common components. The websocket-common module is available to be used by multiple implementations of websocket (ee8, ee9, ee10 etc.). However because it has internal components we have to just through some JPMS hoops with a module-info.java like:

    // The Jetty & Jakarta API Layers need to access both access some internal utilities which we don't want to expose.
    exports org.eclipse.jetty.websocket.core.internal.util to
        org.eclipse.jetty.ee8.websocket.jetty.common,
        org.eclipse.jetty.ee8.websocket.jetty.client,
        org.eclipse.jetty.ee8.websocket.jetty.server,
        org.eclipse.jetty.ee8.websocket.javax.common,
        org.eclipse.jetty.ee8.websocket.javax.client,
        org.eclipse.jetty.ee8.websocket.javax.server,
        org.eclipse.jetty.ee9.websocket.jetty.common,
        org.eclipse.jetty.ee9.websocket.jetty.client,
        org.eclipse.jetty.ee9.websocket.jetty.server,
        org.eclipse.jetty.ee9.websocket.jakarta.common,
        org.eclipse.jetty.ee9.websocket.jakarta.client,
        org.eclipse.jetty.ee9.websocket.jakarta.server,
        org.eclipse.jetty.ee10.websocket.jetty.common,
        org.eclipse.jetty.ee10.websocket.jetty.client,
        org.eclipse.jetty.ee10.websocket.jetty.server,
        org.eclipse.jetty.ee10.websocket.jakarta.common,
        org.eclipse.jetty.ee10.websocket.jakarta.client,
        org.eclipse.jetty.ee10.websocket.jakarta.server;

So this is making these common components available ONLY to our own packages. This feels a bit restrictive for a project that prides itself on being open, flexible and extensible. Essentially what we are saying is that if somebody wanted to create their own app environment with websocket support, then they cannot use our common components to do so. Except that they can, by the terms of our license. It is just now that they will need to fork the project and modify at least the module-info so that they can use our common code to extend websocket in the way that they wish.

I understand the desire to flag these APIs as not fully public - ie we reserve the right to change them. But this feels a bit too restrictive to me.

@sbordet your thoughts?

@lachlan-roberts
Copy link
Contributor Author

This is a discussion for issue #9233 and not this PR.

But these components are never exposed, so there is no way for anyone to plug in and use them. Someone could design their own API layer on top of WebSocket core and use these as utilities to implement that, but its not possible to just extend these classes and plug them in to tweak some behaviour.

@lachlan-roberts lachlan-roberts merged commit 1144d3a into jetty-12.0.x Feb 3, 2023
@joakime joakime deleted the jetty-12.0.x-websocket-core-server-jpms branch March 29, 2023 14:30
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.

Cleanup of websocket in Jetty-12
4 participants