CCDM: consider @PWA annotation when clientSideMode

[manolo]

Fixes #6533

This should not be merged until the target branch PR #7050 has been merged. Then this needs to be re-targeted to cdm

---

This change is 

[vlukashov]

Reviewed 6 of 6 files at r1, 1 of 1 files at r2.
Reviewable status: 1 unresolved discussion, 0 of 1 LGTMs obtained (waiting on @manolo and @platosha)

a discussion (no related file):
I am concerned about the dependency graph that we are creating here.
Ideally, I'd like to move towards separating stateful Flow parts, and stateless Connect parts apart. This implementation encourages re-using parts of the stateful BootstrapHandler class in the stateless IndexHtmlRequestHandler class (see the discussion in Slack: https://vaadin.slack.com/archives/CFP1FJHDH/p1575027106067800?thread_ts=1573646642.024900&cid=CFP1FJHDH)

I am also concerned about having two separate 'registry' classes: VaadinAppShellRegistry for @Meta, @Inline, @BodySize, Viewport etc, and PwaRegistry for @PWA. I would prefer to have a single registry class, with a single modifyIndexHtmlResponse() method that applies modifications from all annotations (including the PWA-related features). Sure, the class itself may (and should) have modular structure with separate handlers for each annotation type.

However, I do not see obvious steps to change this PR to address these concerns. That's why this comment is only for information (non-blocking).

@manolo, do you see any ways how this PR can feasibly be changed to better fit the LT vision?

---

[manolo]

Reviewable status: 1 unresolved discussion, 0 of 1 LGTMs obtained (waiting on @manolo and @platosha)

a discussion (no related file):

Previously, vlukashov (Viktor Lukashov) wrote…

I am concerned about the dependency graph that we are creating here.
Ideally, I'd like to move towards separating stateful Flow parts, and stateless Connect parts apart. This implementation encourages re-using parts of the stateful BootstrapHandler class in the stateless IndexHtmlRequestHandler class (see the discussion in Slack: https://vaadin.slack.com/archives/CFP1FJHDH/p1575027106067800?thread_ts=1573646642.024900&cid=CFP1FJHDH)

I am also concerned about having two separate 'registry' classes: VaadinAppShellRegistry for @Meta, @Inline, @BodySize, Viewport etc, and PwaRegistry for @PWA. I would prefer to have a single registry class, with a single modifyIndexHtmlResponse() method that applies modifications from all annotations (including the PWA-related features). Sure, the class itself may (and should) have modular structure with separate handlers for each annotation type.

However, I do not see obvious steps to change this PR to address these concerns. That's why this comment is only for information (non-blocking).

@manolo, do you see any ways how this PR can feasibly be changed to better fit the LT vision?

well, actually IndexHtmlRequestHandler is coupled to BootstrapHandler because one extends another but index handler does not do any call to the instance of regular bootstrap, just share code.

At some point we should simplify bootstrap and extract all share stuff to a utility class, but I think it should be done when removing bower.

PwaRegistry is needed because it's used by servlets in order to dispatch manifest, icons and serviceworker requests.

When we have our own implementation of PWA where everything is is static in frontend or webapp folders I would think on not depending on this.

---

[vlukashov]

Reviewed 1 of 1 files at r3.
Reviewable status: 1 unresolved discussion, 1 of 1 LGTMs obtained (waiting on @manolo and @platosha)

[vaadin-bot]

SonarQube analysis reported 4 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. VaadinAppShellInitializer.java#L84: Refactor this method to reduce its Cognitive Complexity from 17 to the 15 allowed.
2. BootstrapHandler.java#L552: Remove this use of "BootstrapPageResponse"; it is deprecated.
3. BootstrapHandler.java#L552: Remove this use of "BootstrapPageResponse"; it is deprecated.
4. BootstrapHandler.java#L556: Remove this use of "modifyBootstrapPage"; it is deprecated.

[vlukashov]

Reviewed 6 of 6 files at r4.
Reviewable status: all discussions resolved, 0 of 1 LGTMs obtained, and 1 stale (waiting on @platosha)

[vlukashov]

Reviewable status: complete! all discussions resolved, 1 of 1 LGTMs obtained (waiting on @platosha)