Flow initial pwa #4334

mikotin · 2018-06-27T07:06:38Z

Initial version of PWA.

Sets up simple PWA stack (manifest, service worker, icons, offline handling) for flow application. Configurable via PWA -annotation.

Initial commit: - Unstable - Quick'n'dirty - Works

Fixed code and tests to pass tests. Removed initializer - registry created as singleton on demand. Added support for free size icons.

…ality.

caalador · 2018-06-28T05:24:56Z

flow-server/src/main/resources/META-INF/services/javax.servlet.ServletContainerInitializer

@@ -1,4 +1,4 @@
 com.vaadin.flow.server.startup.ServletVerifier
 com.vaadin.flow.server.startup.RouteRegistryInitializer
 com.vaadin.flow.server.startup.ErrorNavigationTargetInitializer
-com.vaadin.flow.server.startup.AnnotationValidator
+com.vaadin.flow.server.startup.AnnotationValidator


Revert file. Now the only change is that we don't have a new line in the file.

caalador · 2018-06-28T05:27:55Z

flow-server/src/main/java/com/vaadin/flow/dom/Icon.java

+     * @param height height of icon
+     * @return
+     */
+    public Icon size(int width, int height) {


setSize instead of size

Aye, there are chaining setters used in Icon with just property name for each did on purpose. Using setFoo with chaining seems not so good. I'll change those of course if needed. For instance to withFoo if it would be better? Or remove chaining totally, if it doesn't suite.

caalador · 2018-06-28T05:28:25Z

flow-server/src/main/java/com/vaadin/flow/dom/Icon.java

+        return element;
+    }
+
+    private Icon attr(String key, String value) {


For readability of code. addAttribute

caalador · 2018-06-28T05:29:23Z

flow-server/src/main/java/com/vaadin/flow/dom/Icon.java

+     *
+     * @return a String as [size]x[size]
+     */
+    public String sizes() {


caalador · 2018-06-28T05:29:55Z

flow-server/src/main/java/com/vaadin/flow/dom/Icon.java

+     * @param href href
+     * @return self
+     */
+    public Icon href(String href) {


caalador · 2018-06-28T06:22:12Z

flow-server/src/main/java/com/vaadin/flow/server/PwaConfiguration.java

+     * @return
+     */
+    public List<String> getOfflineResources() {
+        return offlineResources.stream().collect(Collectors.toList());


Collections.unmodifiableList(offlineResources)

caalador · 2018-06-28T06:25:33Z

flow-server/src/main/java/com/vaadin/flow/server/VaadinService.java

@@ -292,7 +295,10 @@ public void init() throws ServiceException {
        handlers.add(new UidlRequestHandler());
        handlers.add(new UnsupportedBrowserHandler());
        handlers.add(new StreamRequestHandler());
-
+        PWARegistry pwaRegistry = getPwaRegistry();


The default implementation in VaadinServletService may return null.

Null checking added

caalador · 2018-06-28T06:26:38Z

flow-server/src/main/java/com/vaadin/flow/server/VaadinServletService.java

+                this.pwaRegistry = PWARegistry
+                        .initRegistry(getServlet().getServletContext());
+            } catch (IOException e) {
+                return null;


Instead of returning null throw a new NamedException extends RuntimeException with information on what failed and why as now things will fail with a NPE at some point.

caalador · 2018-06-28T06:37:45Z

flow-server/src/main/java/com/vaadin/flow/server/communication/PWAHandler.java

+        requestHandlerMap = new HashMap<>();
+        this.pwaRegistry = pwaRegistry;
+
+        if (this.pwaRegistry.getPwaConfiguration().isEnabled()) {


if and contents could be in a init() with a direct return if not enabled for clearer reading.

caalador · 2018-06-28T06:38:48Z

flow-server/src/test/java/com/vaadin/flow/server/BootstrapHandlerTest.java

@@ -1018,18 +1018,20 @@ public void theme_contents_are_appended_to_head()



Test changes do not add test for pwa.

Yes, tests are missing. Need to add those.

tests added

Added missing periods Use defined constant Fixed typo (extra ") from meta tag value Changed to use unmodifiableList

caalador

On the fluent API I guess it's fine, but at least the getters should be in the getXYZ format.

caalador · 2018-06-28T08:56:15Z

flow-server/src/main/java/com/vaadin/flow/server/PWA.java

+ *
+ * Only 1 annotation for application is supported.
+ *
+ * Annotation must be placed to master layout.


There's another problem also. Now if you for some reason (Forgetting a PWA when copying or something) have multiple PWA annotations then one of them will be selected and you can't really know which one, might even be a different one each time if you are unlucky.

Going on vacation. Open for picks.

SomeoneToIgnore · 2018-07-05T11:32:16Z

flow-server/src/main/java/com/vaadin/flow/server/PWA.java

+     * Defines the developers’ preferred display mode for the website.
+     *
+     * Possible values:
+     * fullscreen, standalone, minimal-ui, browser


Have you considered making this an enum?

SomeoneToIgnore · 2018-07-05T11:34:08Z

flow-server/src/main/java/com/vaadin/flow/server/PWA.java

+     *
+     * @return Is PWA injecting enabled
+     */
+    boolean enabled() default true;


Is there a use case when someone adds the @PWA annotation but with enabled=false?

No there's not. That was leftover from old code. Removed.

I forgot to publish my review in time. Sorry, new UI of GitHub review.

SomeoneToIgnore · 2018-07-05T11:37:37Z

flow-server/src/main/java/com/vaadin/flow/server/PWAIcon.java

+    private long fileHash = 0;
+    private String baseName = "icons/icon.png";
+    private Domain domain = Domain.HEADER;
+    private boolean hrefOverride = false;


Why do we need hrefOverride? It's not set anywhere.

tag is also used in one place only.

Also basename and domain initial values are not used ever: those are always overwritten in constructor.
Looks suspicious.

Leftovers from old code, removed

SomeoneToIgnore · 2018-07-05T11:47:47Z

flow-server/src/main/java/com/vaadin/flow/server/PWAIcon.java

+     * @param outputStream output stream
+     * @throws IOException possible exception in stream writing
+     */
+    public synchronized void write(OutputStream outputStream)


Any reason to make this synchronized?

SomeoneToIgnore · 2018-07-05T11:51:47Z

flow-server/src/main/java/com/vaadin/flow/server/PWAInitializationException.java

+ * Exception thrown when {@link PWARegistry} initialization fails.
+ *
+ */
+public class PWAInitializationException extends RuntimeException {


I see it's used in one place only and it's our code.
Does it make any sense to have this exception? Can we just throw UncheckedIOException there instead?

SomeoneToIgnore · 2018-07-05T12:24:56Z

flow-server/src/main/java/com/vaadin/flow/server/PwaConfiguration.java

+            this.offlinePath = checkPath(pwa.offlinePath());
+            this.display = pwa.display();
+            this.startUrl = getStartUrl(servletContext);
+            this.serviceWorkerPath = "sw.js";


It's the same in both cases, is it ok?

Leftover from when it was supposed to be configured, but it's a bit dangerous to let it be configured, so it was removed. I'll change it away from different blocks, for that's fixed value.

SomeoneToIgnore · 2018-07-05T12:28:36Z

flow-server/src/main/java/com/vaadin/flow/server/PwaConfiguration.java

+        if (pwa != null) {
+            this.appName = pwa.name();
+            this.shortName = pwa.shortName().substring(0,
+                    Math.min(pwa.shortName().length(), 12));


Should we warn the user that he did not manage to fit into 12 symbols?

SomeoneToIgnore · 2018-07-05T12:30:51Z

flow-server/src/main/java/com/vaadin/flow/server/PwaConfiguration.java

+     *
+     * @return Path to logo with prefix, so request matches
+     */
+    public String relLogoPath() {


Don't really understand why do we need two similar methods.
Can we leave only one and inline this instead, since it's used in a single place only?

Same about other rel prefixed methods here.

rel -prefix was added there to move logic of getting name in object, in stead of adding that logic (very simple, though) to place where needed. I liked the idea of leaving logic to the object. Sure it can be removed, if it doesn't feel right.

SomeoneToIgnore · 2018-07-05T12:39:33Z

flow-server/src/main/java/com/vaadin/flow/server/PWARegistry.java

+        }
+
+        // Google Workbox import
+        stringBuilder.append("importScripts('https://storage.googleapis.com/"


Does this mean we depend on some external CDN resource now?
Is it safe? Afaik we did not do such thing before.

Workbox files (needed ones) are now served internally via flow

denis-anisimov · 2018-07-03T05:21:49Z

flow-server/src/main/java/com/vaadin/flow/server/BootstrapHandler.java

@@ -701,6 +702,42 @@ private static void setupMetaAndTitle(Element head,
        });
    }

+    private static void setupPWA(Element head) {
+        PWARegistry registry = VaadinService.getCurrent().getPwaRegistry();


VaadinService.getCurrent()
Please don't do this.
Artur made an effort to avoid getCurrent() methods call whenever it's possible.

Please pass BootstrapContext context as a method parameter and use it to get VaadinSession and a service from it. There are examples in the methods of this class.

Done by Mikko.

denis-anisimov · 2018-07-03T05:31:15Z

flow-server/src/main/java/com/vaadin/flow/server/PWA.java

+ */
+@Retention(RetentionPolicy.RUNTIME)
+@Target(ElementType.TYPE)
+public @interface PWA {


There are a lot of methods (attributes) in this annotation.
So in the worst case the usage of this annotation will look like
@PWA(offlinePath="abc", manifestPath="foo", logoPath="bar", name="sdfsdf", shortName="cccc",...........................)

Looks inconvenient to me.

I'm not sure exactly how it may be improved.
But may be use PWA as an enabler and delegate all the properties to the class like it's done for @Theme annotation?

I would change in this case PWA to EnablePWA and add class or interface which contains all the methods from here (let's say its name is PWAConfig).
Then EnablePWA would be:

public @interface EnablePWA { Class<? extends PWAConfig> value(); }

Then there will be an additional responsibility to instantiate the provided class

Aye, there are quite a lot of those properties. But I like the developer experience of having all that in that annotation: that way adding values and/or checking default values is supported directly with IDE.

denis-anisimov · 2018-07-03T05:32:31Z

flow-server/src/main/java/com/vaadin/flow/server/PWAIcon.java

+    private Map<String, String> attributes = new HashMap<>();
+    private String tag = "link";
+
+    protected PWAIcon(int width, int height, String baseName) {


I think SQ requires Javadocs for every protected method.

By the way why there is no any public CTOR?
Is it intended to be used internally only (then may be package local?).

All of them are used internally only, have changed to package local.
Just to note, there were arguments about using package local in our code that ended up with using protected instead. But I like package local more.

denis-anisimov · 2018-07-03T05:40:20Z

flow-server/src/main/java/com/vaadin/flow/server/PWAIcon.java

+     * @param outputStream output stream
+     * @throws IOException possible exception in stream writing
+     */
+    public synchronized void write(OutputStream outputStream)


Any specific reason why this method is synchronized ?
I don't see any reason from the code.
The instance of PWAIcon is not intended to be shared between threads (otherwise it requires special design for that which it doesn't have at the moment).

This is the only synchronized method and synchronization is done against the PWAIcon instance.
If external client of this class decides to use it in concurrent code then it's the client responsibility to make synchronization. And this synchronization will be done with totally different monitor (object to lock via synchronization) than PWAIcon instance.
So please remove it.

No reason whatsovere. Was left over from some initial steps of creating PWA. Good spot. Removed.

denis-anisimov · 2018-07-03T05:42:24Z

flow-server/src/main/java/com/vaadin/flow/server/PwaConfiguration.java

+    private final boolean enabled;
+    private List<String> offlineResources;
+
+    protected PwaConfiguration(PWA pwa, ServletContext servletContext) {


So here you copy values from annotation to the PwaConfiguration instance class.
So I would say this class should be used directly as I suggested in PWA.
It also will allow to avoid big if-else since yo may provide default method implementations.

True, initial version of this used that, but @interface is somewhat weak on what it can do (clearing up data and have logic and such). But yes, everything from PwaConfiguration could be moved directly to PwaRegistry and use just PWA internally. Configuration was done with to get data of PWA annotation nicely in an object. I liked the separation, but sure, it's not needed. But I still prefer having it separately instead of pouring everything to PwaRegirsty

No, I meant the other way around......
You are using PWA annotation and it has a huge number of methods. Instead you may require only value which will be PWAConfig subclass and this subclass should be implemented by the developer and set as a value.

Not sure what to do here. I prefer an annotation with parametetrs approach over the class one and it seems like you've also agreed with it a few notes up.

I just explained the original idea here to avoid misunderstanding.

I think that a lot of parameters in the annotation makes it inconvenient to use but that's an opinion which needs a DX.
Do not block this.

denis-anisimov · 2018-07-03T06:11:58Z

flow-server/src/main/java/com/vaadin/flow/server/communication/PWAHandler.java

+                (session, request, response) -> {
+                    response.setContentType("text/html");
+                    response.getWriter().write(pwaRegistry.getOfflineHtml());
+                    response.getWriter().close();


Same here (actually I think there are places above where close is also not inside finally) and about lambda.

Done by Mikko.

denis-anisimov · 2018-07-03T06:11:59Z

flow-server/src/main/java/com/vaadin/flow/server/communication/PWAHandler.java

+                        }
+                        OutputStream out = response.getOutputStream();
+                        icon.write(out);
+                        out.close();


I'm not sure does icon.write declares any throws but event if it doesn't there is always a chance to get RuntimeException .
All streams should be closed anyways. So close should always be in finally block.
And may be extract the lambda as a method reference because it's hard to read lambda inside for.

Done by Mikko.

denis-anisimov · 2018-07-03T06:12:08Z

flow-server/src/main/java/com/vaadin/flow/server/communication/PWAHandler.java

+                    response.setContentType("application/json");
+                    PrintWriter writer = response.getWriter();
+                    writer.write(this.pwaRegistry.getManifestJson());
+                    writer.close();


Done by Mikko.

denis-anisimov · 2018-07-03T06:12:15Z

flow-server/src/main/java/com/vaadin/flow/server/communication/PWAHandler.java

+                    response.setContentType("application/javascript");
+                    PrintWriter writer = response.getWriter();
+                    writer.write(this.pwaRegistry.getServiceWorkerJs());
+                    writer.close();


Done by Mikko.

denis-anisimov · 2018-07-03T06:14:23Z

flow-server/src/main/java/com/vaadin/flow/server/startup/RouteRegistry.java

+
+    protected void setPwaClass(Class<?> pwaClass) {
+        if (pwaClass != null && pwaClass.isAnnotationPresent(PWA.class)) {
+            this.pwaConfigurationClass = pwaClass;


That needs to be atomic since the method is called from the other thread than getter.
Use AtomicReference for the pwaConfigurationClass.

Done by Mikko.

…flow-initial-pwa

SomeoneToIgnore · 2018-07-10T09:40:56Z

@denis-anisimov I've looked through your last comments and fixed everything that was missing here.

denis-anisimov

There are a couple of places where StringBuilder is used but with string concatenation.
Everything else is fine.

denis-anisimov · 2018-07-11T06:18:51Z

flow-server/src/main/java/com/vaadin/flow/server/PwaRegistry.java

+        String workBoxAbsolutePath = servletContext.getContextPath() + "/"
+                + WORKBOX_FOLDER;
+        // Google Workbox import
+        stringBuilder.append("importScripts('" + workBoxAbsolutePath


string concatenation within string builder append method looks weird.

Replaced with append calls.

vaadin-bot · 2018-07-11T08:27:10Z

SonarQube analysis reported 7 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:

VaadinService.java#L2048: First sentence of Javadoc is incomplete (period is missing) or not present.
VaadinService.java#L2082: First sentence of Javadoc is incomplete (period is missing) or not present.
VaadinService.java#L2094: First sentence of Javadoc is incomplete (period is missing) or not present.
VaadinService.java#L2106: First sentence of Javadoc is incomplete (period is missing) or not present.
RouteRegistry.java#L577: A "NullPointerException" could be thrown; "navigationTarget" is nullable here.
VaadinService.java#L521: This call to "add()" may be a performance hot spot if the collection is large.
VaadinService.java#L522: This call to "remove()" may be a performance hot spot if the collection is large.

mikotin added 12 commits June 19, 2018 18:16

Adds automatic PWA -initialization for all Flow-projects.

74ccbf0

Initial commit: - Unstable - Quick'n'dirty - Works

Renamed Manifest to PWA.

110a643

Fixed code and tests to pass tests. Removed initializer - registry created as singleton on demand. Added support for free size icons.

Added missing type from icons

b03bdb0

Added missing href setting

1953c59

Changed pwa handlers to a single handler with pre-calculated responses.

3538cbf

Fixed icon inject for offline page.

8908c3b

Updated pwa properties to better support add to home screen -function…

fc2a561

…ality.

Cleared up the code and added comments.

f52538a

Added support for custom offline resources.

5b0b1cc

Added missing documentation, moved PWARegistry to server -package.

10c19f1

Added required docs and fixed minor issues reported by bot.

97d580c

Added meta tags for better PWA support.

efeb58e

caalador previously requested changes Jun 28, 2018

View reviewed changes

mikotin added 3 commits June 28, 2018 10:32

Fixed commented issues.

e19249a

Added missing periods Use defined constant Fixed typo (extra ") from meta tag value Changed to use unmodifiableList

Fixed comments.

b942b51

Fixed comments.

45f9931

caalador reviewed Jun 28, 2018

View reviewed changes

Refactored Icon class and added missing copyright headers.

96451c3

mikotin added 11 commits June 29, 2018 14:35

Moved PWA scanning to route registry initializer.

11454ad

Fixed reported issues

3c4a794

Changed general exception to a named exception.

e0ab2da

Removed unused import.

067fb12

Replaced repeated strings with constants.

2185806

Minor code style change. Added tesource stream close handling.

a5ab9fe

Changed resource reading as informed by bot.

c8b77e2

Added missing copyright header.

a73ebe4

Added tests for PWA annotation.

9a44794

Changed default logo for pwa

f7ad12c

Default display changed to standalone and updated default resources.

d5d926f

SomeoneToIgnore suggested changes Jul 5, 2018

View reviewed changes

Remove unneeded CSS property.

15a789f

denis-anisimov suggested changes Jul 5, 2018

View reviewed changes

mikotin added 11 commits July 6, 2018 12:16

Added local serving of needed Workbox -files.

663cd54

Merge branch 'flow-initial-pwa' of ssh://github.com/vaadin/flow into …

c71e058

…flow-initial-pwa

Removed enabled from PWA annotation.

682c3ad

Cleared silly issues that were left in the code.

0d67035

Moved fixed value out of if clause.

efb7050

Replaced getCurrent() with proper way.

b2bd0cd

Added synchronized for to lazy initializator.

3612a12

Removed unused import.

38c0460

Changed private field to AtomitRefence.

e1fe75d

Updated version.

b4c69d0

Fixed constant.

ff85742

SomeoneToIgnore approved these changes Jul 10, 2018

View reviewed changes

Kirill Bulatov added 3 commits July 10, 2018 11:15

Merge branch 'master' into flow-initial-pwa

34eeaa7

Fix sonar issues.

6af74cd

Code review fixes.

4da44a9

Fix NPE.

689730a

SomeoneToIgnore force-pushed the flow-initial-pwa branch from b96f49f to 689730a Compare July 10, 2018 12:36

denis-anisimov suggested changes Jul 11, 2018

View reviewed changes

Replace string concatenation with append.

a55390a

denis-anisimov approved these changes Jul 11, 2018

View reviewed changes

vaadin deleted a comment from denis-anisimov Jul 11, 2018

vaadin deleted a comment from SomeoneToIgnore Jul 11, 2018

SomeoneToIgnore merged commit 022dd44 into master Jul 11, 2018

SomeoneToIgnore deleted the flow-initial-pwa branch July 11, 2018 08:30

gilberto-torrezan added this to the 1.1.0.alpha1 milestone Jul 18, 2018

		@@ -1018,18 +1018,20 @@ public void theme_contents_are_appended_to_head()

Flow initial pwa #4334

Flow initial pwa #4334

Conversation

mikotin commented Jun 27, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

caalador left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SomeoneToIgnore commented Jul 10, 2018

denis-anisimov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vaadin-bot commented Jul 11, 2018