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

CCDM: deprecate server-side bootstrap related APIs #6517

Merged
merged 2 commits into from Sep 25, 2019

Conversation

qtdzz
Copy link
Contributor

@qtdzz qtdzz commented Sep 20, 2019

Fixes #6484
This PR deprecates BootstrapListener related APIs in favor of ClientIndexBootstrapListener.

For other APIs which were used to modify/add content to the bootstrap page (e.g. @PWA, PageConfigurator, @Inline,....) need to be considered separately.


This change is Reviewable

@qtdzz qtdzz added the hilla Issues related to Hilla label Sep 20, 2019
@qtdzz qtdzz force-pushed the tn/ccdm/deprecate-normal-bootstrap-api branch from c80b2fa to cda6f23 Compare September 23, 2019 09:09
* @author Vaadin Ltd
* @since 1.0
*/
@FunctionalInterface
@Deprecated
public interface BootstrapListener extends EventListener, Serializable {
Copy link
Collaborator

Choose a reason for hiding this comment

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

INFO Do not forget to remove this deprecated code someday. rule

*/
@Deprecated
public void modifyBootstrapPage(BootstrapPageResponse response) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

INFO Do not forget to remove this deprecated code someday. rule

* @author Vaadin Ltd
* @since 1.0
*/
@Deprecated
public class BootstrapPageResponse {
Copy link
Collaborator

Choose a reason for hiding this comment

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

INFO Do not forget to remove this deprecated code someday. rule

@ujoni ujoni added the +0.1.0 label Sep 23, 2019
@qtdzz qtdzz force-pushed the tn/ccdm/deprecate-normal-bootstrap-api branch from cda6f23 to 6d298ec Compare September 24, 2019 10:48
Copy link
Contributor

@platosha platosha left a comment

Choose a reason for hiding this comment

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

Reviewed 3 of 3 files at r1.
Reviewable status: 3 unresolved discussions, 0 of 1 LGTMs obtained (waiting on @qtdzz)

a discussion (no related file):
BTW Not sure if this is relevant, but I find a few usages of BootstrapListener around, e. g., ServiceInitEvent#addBootstrapListener and so on. I guess this is kind of deprecated automatically, no need for annotation?


OLD Vaadin Flow ongoing work (Vaadin 10+) automation moved this from External Reviews to Reviewer approved Sep 24, 2019
Copy link
Contributor Author

@qtdzz qtdzz left a comment

Choose a reason for hiding this comment

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

Reviewable status: 3 unresolved discussions, 0 of 1 LGTMs obtained (waiting on @platosha and @qtdzz)

a discussion (no related file):

Previously, platosha (Anton Platonov) wrote…

BTW Not sure if this is relevant, but I find a few usages of BootstrapListener around, e. g., ServiceInitEvent#addBootstrapListener and so on. I guess this is kind of deprecated automatically, no need for annotation?

Good catch! I actually missed them. I added the annotation and reword the message to be more descriptive.


*/
@Deprecated
default Stream<BootstrapListener> getBootstrapListeners(
Copy link
Collaborator

Choose a reason for hiding this comment

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

INFO Do not forget to remove this deprecated code someday. rule

*/
@Deprecated
public void addBootstrapListener(BootstrapListener bootstrapListener) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

INFO Do not forget to remove this deprecated code someday. rule

*/
@Deprecated
public Stream<BootstrapListener> getAddedBootstrapListeners() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

INFO Do not forget to remove this deprecated code someday. rule

@vaadin-bot
Copy link
Collaborator

SonarQube analysis reported 10 issues

  • MINOR 4 minor
  • INFO 6 info

Watch the comments in this conversation to review them.

4 extra issues

Note: The following issues were found on lines that were not modified in the pull request. Because these issues can't be reported as line comments, they are summarized here:

  1. MINOR ServiceInitEvent.java#L40: Remove this use of "BootstrapListener"; it is deprecated. rule
  2. MINOR VaadinService.java#L176: Remove this use of "BootstrapListener"; it is deprecated. rule
  3. MINOR VaadinService.java#L287: Remove this use of "getBootstrapListeners"; it is deprecated. rule
  4. MINOR VaadinService.java#L287: Remove this use of "getAddedBootstrapListeners"; it is deprecated. rule

Copy link
Contributor

@platosha platosha left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 5 of 5 files at r3.
Reviewable status: 6 unresolved discussions, 1 of 1 LGTMs obtained (waiting on @qtdzz)

Copy link
Contributor Author

@qtdzz qtdzz left a comment

Choose a reason for hiding this comment

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

Dismissed @vaadin-bot from 6 discussions.
Reviewable status: :shipit: complete! all discussions resolved, 1 of 1 LGTMs obtained

@qtdzz qtdzz merged commit 335c350 into ccdm Sep 25, 2019
OLD Vaadin Flow ongoing work (Vaadin 10+) automation moved this from Reviewer approved to Done - pending release Sep 25, 2019
@qtdzz qtdzz deleted the tn/ccdm/deprecate-normal-bootstrap-api branch September 25, 2019 12:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hilla Issues related to Hilla +0.1.0
Projects
OLD Vaadin Flow ongoing work (Vaadin ...
  
Done - pending release
Development

Successfully merging this pull request may close these issues.

None yet

4 participants