search

PolymerElements / iron-iconset-svg

Represents a source of icons expressed as a collection of inline SVGs
https://www.webcomponents.org/element/PolymerElements/iron-iconset-svg
37 stars 34 forks source link

Fix: Large iconsets rendering #79

Closed stramel closed 6 years ago

stramel commented 6 years ago

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 commented 6 years ago

LGTM

stramel commented 6 years ago

@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 commented 6 years ago

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

<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>
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.)

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 commented 6 years ago

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 commented 6 years ago

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

tiwijo commented 6 years ago

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 commented 6 years ago

@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 commented 6 years ago

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.