Fix: Large iconsets rendering by stramel · Pull Request #79 · PolymerElements/iron-iconset-svg

stramel · 2018-01-23T20:27:44Z

Fixes #66

When just using this.async with a large import tree, this._createIconMap() returns an empty object.
When using Polymer.RenderStatus.afterNextRender, this._createIconMap() returns the expected icons.

This feels like a better solution than waiting for the entire document#DOMContentLoaded event suggested in #77

Will add tests and update changelog after some more testing and initial feedback.

jay8t6 · 2018-01-23T21:19:07Z

LGTM

haydenmcp · 2018-01-23T21:47:54Z

-      this.async(function() {
+      // Give time for icons to be added
+      Polymer.RenderStatus.afterNextRender(this, () => {
        this.fire('iron-iconset-added', this, {node: window});


Is this a 2.x legacy version? If so, maybe use this.dispatchEvent( ... );

Polymer team's elements are hybrid and for the most part are using this.fire. I assume in the refactor of their elements they will switch to using this.dispatchEvent(new CustomEvent())

stramel · 2018-01-25T21:51:37Z

@bicknellr Would you be open to this change? If you would like, I can try to figure out how to test this as well.

bicknellr · 2018-01-27T00:05:20Z

I tried reproducing this and here's what I ended up with.

Put this in demo/:

<html>
<head>
  <script src="../../webcomponentsjs/webcomponents-lite.js"></script>
  <link rel="import" href="./other-icons.html" async>
  <link rel="import" href="../../iron-icon/iron-icon.html" async>
  <style>
    iron-icon {
      border: 1px solid gray;
      width: 48px;
      height: 48px;
    }
  </style>
</head>
<body>
  <iron-icon icon="other:shape1"></iron-icon>
  <iron-icon icon="other:shape10"></iron-icon>
  <iron-icon icon="other:shape100"></iron-icon>
  <iron-icon icon="other:shape1000"></iron-icon>
  <iron-icon icon="other:shape10000"></iron-icon>
  <iron-icon icon="other:shape16000"></iron-icon>
  <iron-icon icon="other:shape32000"></iron-icon>
</body>
</html>

Run this script (python 3) from within demo/ to generate other-icons.html:

import math
import random

f = open('other-icons.html', 'w')
f.write('''
<link rel="import" href="../iron-iconset-svg.html">

<iron-iconset-svg name="other" size="48">
  <svg>
    <defs>
''')
for i in range(0, 2 ** 15):
  f.write('<g id="shape' + str(i) + '">')
  f.write('<text x="0" y="24" font-size="10">' + str(i) + '</text>')
  f.write('</g>\n')
  f.write('<!--')
  # defeat compression w/ randomness because i'm too lazy to figure out how to disable it in polyserve...
  f.write(''.join([chr(65 + math.floor(26 * random.random())) for i in range(0, 2 ** 10)]))
  f.write('-->\n')
f.write('''
    </defs>
  </svg>
</iron-iconset-svg>
''')
f.close()

(other-icons.html ends up being about ~36MB and contains ~32k icons with a bit over 1KB stuffed between each. gzip seems to compress this to ~20MB. I ended up going down this "make the iconset huge" rabbit hole because I couldn't reproduce just by using a more reasonably sized icon set and simulating a really slow network. Also, I'm beginning to doubt how heavily compression factored into increasing the amount of data needed to cause the page to be broken up (?) since parsing time shouldn't be affected by compression. The comments are probably being parsed dramatically faster than anything else would be, so I'll try generating lots of extra icons rather than comments next.)

polymer serve and navigate to the page you made first.

When I go to that page, the icons up through other:shape10000 load but the others don't. Breaking at the end of _createIconMap usually shows that around 10-15k icons end up in the map; so not the complete set, just as the bug mentioned. Something to note though: the async attributes on those imports is necessary for me to reproduce this. More importantly, applying the proposed changes from this PR doesn't fix this issue for me.

Given that the async attribute is necessary to reproduce this, it seems like an ordering issue: the <iron-icon>s using icons from the icon set get constructed before the entire icon set is loaded. When an iron-icon gets constructed it immediately finds and requests an icon from an icon set (assuming it has a set icon attribute). At this point, I'm assuming that the icon set has been constructed but some of its icons have not yet been parsed or inserted into its tree; so, when it gets asked to produce an icon for the first time, it collects all its icons but misses a bunch. (Props to @notwaldorf for helping me track this down.)

I'm surprised that the proposed fix here doesn't seem to work in my situation. I suppose rendering incomplete pages is part of the way initial loads work and that might be why afterNextRender(this, ... isn't completely effective here? I can't think of any solutions for this off the top of my head that don't involve requiring users to insert another element after the icon set. :/

Assuming your imports are set up like this example, removing the async attribute from the import containing the icon set should work around this by forcing it to be parsed completely before the iron-icons are. Is anyone that's running into this using the async attribute? If not, are you sure you've imported your icon set in the place you're using it?

Just out of curiosity, how large are the icon sets / bundles we're talking about?

stramel · 2018-01-27T20:27:40Z

I was having the issue with iron-icons. After removing the async attribute it seems to work.

I wonder if the double afterNextRender trick would work? Or maybe it would be good just to add a note that async can cause issues?

bicknellr · 2018-01-30T22:51:07Z

@generictjohnson, are you also using the async attribute? If so, does removing it work for you?

tiwijo · 2018-01-30T23:09:10Z

IIRC, I was not using the async attribute. I think the HTML was getting chopped into pieces by the frontend server (which does chunking), and then an implicit closing tag was causing Polymer to think that it had received all of the icons.

stramel · 2018-02-02T20:59:53Z

@bicknellr Thank you for taking time to help debug this issue. I'm going to close this PR as it doesn't seem to be a proper fix for it. Does seem possibly related to async though. Maybe we can continue more discussion in the issue #66

dlockhart · 2018-02-09T19:51:37Z

I'm also seeing this issue, and am not using async on any of my imports. It only seems to occur with Polymer 2, although that could just be due to different bundle sizes with Polymer 1 vs. 2. Changing my bundle size (by just removing various imports) has the effect of making some iconsets earlier in the import work.

Both the fix in this PR (#79) and #77 solve the issue for me.

Fix: Large iconsets render

af0dd09

googlebot added the cla: yes label Jan 23, 2018

haydenmcp reviewed Jan 23, 2018

View reviewed changes

haydenmcp approved these changes Jan 23, 2018

View reviewed changes

stramel closed this Feb 2, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix: Large iconsets rendering#79

Fix: Large iconsets rendering#79
stramel wants to merge 1 commit into
PolymerElements:masterfrom
stramel:ms/fix-large-iconsets

stramel commented Jan 23, 2018 •

edited

Loading

Uh oh!

jay8t6 commented Jan 23, 2018

Uh oh!

haydenmcp Jan 23, 2018

Uh oh!

stramel Jan 23, 2018

Uh oh!

stramel commented Jan 25, 2018

Uh oh!

bicknellr commented Jan 27, 2018

Uh oh!

stramel commented Jan 27, 2018

Uh oh!

bicknellr commented Jan 30, 2018

Uh oh!

tiwijo commented Jan 30, 2018

Uh oh!

stramel commented Feb 2, 2018

Uh oh!

dlockhart commented Feb 9, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

Conversation

stramel commented Jan 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jay8t6 commented Jan 23, 2018

Uh oh!

haydenmcp Jan 23, 2018

Choose a reason for hiding this comment

Uh oh!

stramel Jan 23, 2018

Choose a reason for hiding this comment

Uh oh!

stramel commented Jan 25, 2018

Uh oh!

bicknellr commented Jan 27, 2018

Uh oh!

stramel commented Jan 27, 2018

Uh oh!

bicknellr commented Jan 30, 2018

Uh oh!

tiwijo commented Jan 30, 2018

Uh oh!

stramel commented Feb 2, 2018

Uh oh!

dlockhart commented Feb 9, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

stramel commented Jan 23, 2018 •

edited

Loading