-
Notifications
You must be signed in to change notification settings - Fork 168
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
refactor!: Remove deployment conf field from VaadinSession #18319
base: main
Are you sure you want to change the base?
Conversation
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 change looks good and makes sense.
A minor drawback will be that it might break some test configurations on other repositories. In MPR for example, it is used in a couple of tests, and the problem here may be that we use the same MPR codebase against different Flow version.
Potentially also user projects may be affected, since setConfiguration()
is a public API, even though I suppose VaadinSession.setConfiguration()
is not widely used.
flow-server/src/main/java/com/vaadin/flow/server/VaadinSession.java
Outdated
Show resolved
Hide resolved
flow-server/src/main/java/com/vaadin/flow/server/VaadinSession.java
Outdated
Show resolved
Hide resolved
Some tests to fix, but other than that LGTM |
Quality Gate passedThe SonarCloud Quality Gate passed, but some issues were introduced. 3 New issues |
Despite approval, do not merge. This should be postponed until major release of Flow (25), since this changes public API. |
Would it be better to make it a draft again, to prevent unwanted merge? |
Fixes #18318