Fix warnings found by the new linter #114

rictic · 2017-03-18T01:02:40Z

Note, this PR is a work in progress. It definitely isn't ready to merge until Polymer/polymer-analyzer#543 has landed and the new imports in shop-app are rel="lazy-import"

keanulee · 2017-03-20T17:58:16Z

src/shop-app.html

@@ -18,11 +18,25 @@
 <link rel="import" href="../bower_components/iron-media-query/iron-media-query.html">
 <link rel="import" href="../bower_components/iron-pages/iron-pages.html">
 <link rel="import" href="../bower_components/iron-selector/iron-selector.html">
+<link rel="import" href="../bower_components/paper-icon-button/paper-icon-button.html">


paper-icon-button is lazily imported by lazy-resources.html and should not be imported here.

This is also the case for the other added imports in this PR.

Fixed here.

Hm, for cases where one element uses another one but depends on its container doing a lazy import of it... that's a bit tricky. How do you capture that pattern declaratively?

The option I went with here was to add a lazy import of lazy-resources.html to each of the pages, as they can be sure that those resources will eventually be imported.

@rictic this should be changed to a lazy import I guess

keanulee · 2017-03-20T18:00:53Z

src/shop-button.html

@@ -72,4 +72,11 @@

    </style>
  </template>
+  <script>
+    // TODO(rictic): how does this work with just a dom-module? Is that kosher?
+    //     https://github.com/Polymer/polymer-analyzer/issues/573


https://github.com/Polymer/polymer-analyzer/issues/573#issuecomment-287845690

Replied there with a proposal.

TimvdLippe · 2017-03-20T19:58:07Z

src/shop-app.html

+  <link rel="import" href="shop-list.html">
+  <link rel="import" href="shop-detail.html">
+  <link rel="import" href="shop-cart.html">
+  <link rel="import" href="shop-checkout.html">


I have voiced my opinion in the #tools channel and in direct messages with you on Slack, that I really dislike this pattern. It essentially requires users to 1. specify the routes in the selector, 2. specify the fragments and 3. specify the lazy-imports. This is not DRY and is very error-prone for the users.

Point 2 of the above list can of course be derived from point 3, but why can't these both be derived from point 1? The problem specifically in the shop app is that it relies on Polymer.Base.importHref which is extremely hard to analyze statically. Therefore I would like to request a solution that is statically analyzable, which is DRY (in the sense that it only requires users to specify the routes in the selector) and user-friendly.

Any sort of hint or pattern for the analyzer to conclude the different fragments that are lazy-loaded would be sufficient, to which it can combine the final document locations together with the routes. I am not aware of any clear solution, which would work for all users, given that the shop lazy-loading pattern is very specific to this application. A lot of other users are probably relying on a different mechanism, but at the moment I do not know all of the patterns that are used.

By fragments do you mean the DOM to display for each possible choice in iron-pages?

IMO the routes, the per-page DOM, and the urls are three separate concerns, so I'm not concerned that all three need to be specified, but I am a little uncomfortable with how they're specified in such different places such that it's easy for them to get out of sync.

IMO your iron-lazy-pages element goes in the right direction, by specifying the per-page dom in the same place that you specify the url that needs to be imported to render that page.

By fragments do you mean the DOM to display for each possible choice in iron-pages?

No I meant the fragments as specified in the polymer.json

Yes I agree that they are separate concerns, which should be specified, but if these are in different places people forget things. We have hit this exact problem multiple times, in which I built custom scripts to fix that. Yes these custom scripts do use iron-lazy-pages, since we are using them, but at least they give more security for us as application developers that stuff doesn't break when we deploy.

I feel that these custom scripts should be integrated somewhere, without too much hassle for the user. Moreover, the same mechanic can be used to analyze the lazy-fragments.

Oh I see. Yeah, that seems very reasonable. Something like, for every fragment in fragments there should be a lazy import of it somewhere in the application?

Actually, I would like to have single point of truth. The fragments are related to how they are imported and in the shop application are directly retrieved from the name of the routes. Therefore, the names of the routes are the single point of truth imo and the other two can be derived from them.

@TimvdLippe just pointed me here, I think making these lazy imports is exactly the right solution. Then we can work towards not requiring the to be also specified as fragments in polymer.json.

frankiefu · 2017-03-21T18:20:10Z

Should this PR be on 2.0-preview branch (or 2.0-preview-class for Class-based syntax)? master is still for Polymer 1.x stuffs.

Found one maybe-legit bug where we were setting the name property instead of the name attribute in an iron-selector with attr-for-selected. Found a few more places where there were missing imports. And finally, found a few places where the analyzer or a lint rule got confused. Filed bugs or sent PRs for all those.

rictic · 2017-05-11T04:48:07Z

Updated to point to 2.0-preview-class

justinfagnani · 2017-05-11T04:46:56Z

src/shop-app.html

@@ -18,11 +18,25 @@
 <link rel="import" href="../bower_components/iron-media-query/iron-media-query.html">
 <link rel="import" href="../bower_components/iron-pages/iron-pages.html">
 <link rel="import" href="../bower_components/iron-selector/iron-selector.html">
+<link rel="import" href="../bower_components/paper-icon-button/paper-icon-button.html">


@rictic this should be changed to a lazy import I guess

justinfagnani · 2017-05-11T04:49:33Z

src/shop-cart.html

@@ -14,7 +14,7 @@
 <link rel="import" href="shop-form-styles.html">

 <dom-module id="shop-cart">
-
+  <link rel="lazy-import" href="lazy-resources.html" group="imported-by-app">


Add a comment, and maybe a todo to restructure lazy imports to be loaded more locally.

stramel · 2017-06-21T03:55:12Z

bower.json

@@ -19,7 +19,8 @@
    "iron-pages": "polymerelements/iron-pages#2.0-preview",
    "iron-selector": "polymerelements/iron-selector#2.0-preview",
    "paper-icon-button": "polymerelements/paper-icon-button#2.0-preview",
-    "paper-spinner": "polymerelements/paper-spinner#2.0-preview"
+    "paper-spinner": "polymerelements/paper-spinner#2.0-preview",
+    "lazy-imports": "Polymer/lazy-imports#master"


v2.0.0 has landed 🎉 Maybe rebasing this would be a good start 👍

stramel · 2017-06-21T03:57:02Z

src/shop-app.html

@@ -281,7 +288,7 @@
                attr-for-selected="name">
              <dom-repeat items="[[categories]]" as="category" initial-count="4">
                <template>
-                <shop-tab name="[[category.name]]">
+                <shop-tab name$="[[category.name]]">


NIT: indent 2 spaces?

stramel · 2017-06-21T04:03:20Z

src/shop-cart-item.html

@@ -14,6 +14,7 @@
 <link rel="import" href="shop-icons.html">
 <link rel="import" href="shop-image.html">
 <link rel="import" href="shop-select.html">
+<link rel="import" href="shop-md-decorator.html">


Couldn't this be added as a lazy-import which won't actually require the fake elements to exist.

stramel · 2017-06-21T04:04:19Z

src/shop-button.html

@@ -72,4 +72,10 @@

    </style>
  </template>
+  <script>


Is this needed for a style module?

stramel · 2017-06-21T04:05:52Z

src/shop-cart.html

-
+  <!--
+    This is lazy imported in shop-app so we can assume that it's lazy imported
+    here too. Maybe this should be loaded more locally though?


👍 For loading more locally

keanulee · 2018-09-13T17:54:31Z

Stale PR. Please make a new one if necessary.

keanulee reviewed Mar 20, 2017

View reviewed changes

TimvdLippe reviewed Mar 20, 2017

View reviewed changes

TimvdLippe mentioned this pull request Apr 4, 2017

Add support for lazy-imports of iron-lazy-pages Polymer/polymer-analyzer#600

Closed

1 task

rictic added 2 commits May 10, 2017 20:55

Use real lazy-imports.

d1d086f

rictic force-pushed the lint-clean branch from 72db689 to d1d086f Compare May 11, 2017 04:14

rictic changed the base branch from master to 2.0-preview-class May 11, 2017 04:15

rictic force-pushed the lint-clean branch 3 times, most recently from 58e24b1 to c0fa87f Compare May 11, 2017 04:43

Address review comments.

a567c47

rictic force-pushed the lint-clean branch from c0fa87f to a567c47 Compare May 11, 2017 04:44

Unify my TODO comments, pointing at linter bug.

b92e96a

justinfagnani reviewed May 11, 2017

View reviewed changes

Add a couple more comments.

221d8c8

stramel reviewed Jun 21, 2017

View reviewed changes

keanulee closed this Sep 13, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix warnings found by the new linter #114

Fix warnings found by the new linter #114

rictic commented Mar 18, 2017

keanulee Mar 20, 2017

rictic May 11, 2017

justinfagnani May 11, 2017

keanulee Mar 20, 2017

rictic May 11, 2017

TimvdLippe Mar 20, 2017

rictic Mar 20, 2017

TimvdLippe Mar 20, 2017

rictic Mar 20, 2017

TimvdLippe Mar 20, 2017

justinfagnani Apr 6, 2017

frankiefu commented Mar 21, 2017

rictic commented May 11, 2017

justinfagnani May 11, 2017

justinfagnani May 11, 2017

rictic May 11, 2017

stramel Jun 21, 2017

stramel Jun 21, 2017

stramel Jun 21, 2017

stramel Jun 21, 2017

stramel Jun 21, 2017

keanulee commented Sep 13, 2018

Fix warnings found by the new linter #114

Fix warnings found by the new linter #114

Conversation

rictic commented Mar 18, 2017

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

frankiefu commented Mar 21, 2017

rictic commented May 11, 2017

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

keanulee commented Sep 13, 2018