Shapes: js not needed #38

FilipChalupa · 2019-06-10T16:29:20Z

Nové nastavení je rychlejší, protože spuštění stažení shapes nečeká na vykonání javascriptu a zároveň dává více informací browseru o tom, co jsou shapes za typ souboru, takže je může lépe cachovat.

(IE celé url v xlink:href nepodporuje, takže pro něj se dál používají staré shapes na základě detekce podle user agentu.)

enzy

I would prefer a sort of a feature detection approach instead. Import and execute when the primary method fails.
There were also other reasons (other than IE) not to use direct path but I can't remember it now.

FilipChalupa · 2019-06-10T19:22:36Z

Same here, however, there isn't a way how to feature detect AFAIK.

Delayed import for IE only for a few lines of code is a bit overkill.

I've tested Chrome, Safari, Firefox, Edge, IE. All seem to work consistently.

JirkaVebr

I completely agree with @enzy that a feature detection based approach would have been preferable but much like @onset, I cannot think of any. 🙁 Can you, @enzy?

Also, now that shapes are being downloaded as an actual image, why don't we add something like <link rel="preload" as="image" href="/assets/shapes.svg"> (or whatever the appropriate value for as is)? I'd say that being able to start downloading early is the just about the main benefit of and motivation for this change so why not go even further? 🙂

src/scripts/components/ShapesIE.ts

FilipChalupa · 2019-06-11T11:36:53Z

Also, now that shapes are being downloaded as an actual image, why don't we add something like <link rel="preload" as="image" href="/assets/shapes.svg"> (or whatever the appropriate value for as is)? I'd say that being able to start downloading early is the just about the main benefit of and motivation for this change so why not go even further? 🙂

Preloading is overoptimization in this case. Hard maintainability is not worth it. Browser can start downloading with first discovered shape in HTML. That can be a logo in header close to head tag where <link rel="preload" would be. What do you think?

JirkaVebr · 2019-06-11T11:53:06Z

overoptimization

As in it would make the site too fast? What do you mean by that?

Hard maintainability is not worth it.

You're already inlining #{assetPath}/sprites/shapes.svg twice in two different files. Turning

script.
	initComponents = [
		{ name: 'ShapesIE', data: { url: '#{assetPath}/sprites/shapes.svg' } }
	]

into

- const shapesUrl = `${assetPath}/sprites/shapes.svg`
  
link(rel='preload', as='image', href=shapesUrl)
script.
	initComponents = [
		{ name: 'ShapesIE', data: { url: '#{shapesUrl}' } }
	]

does not really incur any maintainability overheads.

Now there may be perfectly legitimate reasons not to preload the sprite but I'd say the maintainability aspect you mention is not one of them.

FilipChalupa · 2019-06-11T12:27:37Z

As in it would make the site too fast? What do you mean by that?

Nearly same speed for the cost of harder maintainability.

You're already inlining #{assetPath}/sprites/shapes.svg twice

I'm not happy about it. With IE dead there will be one occurrence only.

Meanwhile, two occurrences are less than three. Keep in mind that mango-cli-example is a very small project. Some can have more than one layout requiring the same rel='preload'/initComponents ... 'ShapesIE' in all of them.

Maintainability is not the main question. What will <link rel="preload" as="image" href="/assets/shapes.svg"> solve? From what I know browsers will discover rel="preload" almost at the same time as the first shape.

enzy · 2019-06-11T13:04:33Z

There were also other reasons (other than IE) not to use direct path but I can't remember it now.

A. It was the Microsoft Edge, which works now ✔️

B. And the more important problem I can see: ❌

SVG references this way has it's own separate DOM. It goes beyond the regular Shadow DOM boundary that all is subject to.... you can't style individual shapes like you could before.
https://css-tricks.com/svg-use-with-external-reference-take-2/#article-header-id-4

/* This won't work either way,
   because it crosses a shadow DOM boundary */
.icon-1 /* ~shadow~ */ .path-1 {
  fill: yellow;
}

enzy

Extract Failing build hotfix outside of this PR please and update it to the latest version (3.1.0 atm). Thank you.

FilipChalupa · 2019-06-11T13:11:50Z

B. And the more important problem I can see: ❌

I didn't know that you can do this with shadow dom. Do we need to support this use case? Shapes are appropriate for simple icons anyway.

FilipChalupa · 2019-06-11T13:14:57Z

Extract Failing build hotfix outside of this PR please and update it to the latest version (3.1.0 atm). Thank you.

I'm ready to drop the commit after some fixes master. I don't have resources to migrate whole project. :(

enzy · 2019-06-11T13:28:38Z

Import and execute when the primary method fails.

Maybe an onload handler would be the way how to detect un/successful attempt. Give it some timeout and inject sprites if load hasn't happened yet.

From IE docs:

The onload event occurs when the browser has fully parsed the element and its descendants and is ready to act appropriately on that element, such as rendering the element to the target device. Before the event occurs, the browser must have loaded, parsed, and prepared referenced external resources for rendering. Optional external resources are not required to be ready for the event to occur.

enzy · 2019-06-11T13:44:28Z

I'm ready to drop the commit after some fixes master. I don't have resources to migrate whole project. :(

Okay, prepared #39 for this.

B. And the more important problem I can see: ❌

I didn't know that you can do this with shadow dom. Do we need to support this use case? Shapes are appropriate for simple icons anyway.

I can think of one example: animating of manGoweb logo would not be possible this way. But I agree that these special cases could use direct inline svg.

FilipChalupa · 2019-06-11T13:49:40Z

Maybe an onload handler would be the way how to detect un/successful attempt. Give it some timeout and inject sprites if load hasn't happened yet.

It's too complicated to implement, test, maintain to support a deprecated browser.
How would timeout help? Why not listen to onerror? Besides, there is a risk of false positives/negatives.

FilipChalupa · 2019-06-11T13:50:31Z

Okay, prepared #39 for this.

🔝 Thank you.

FilipChalupa · 2019-06-11T13:53:45Z

I can think of one example: animating of manGoweb logo would not be possible this way. But I agree that these special cases could use direct inline svg.

So, are we ok with dropping the support of more complex cases since there are other ways to achieve the same like inlining or splitting a shape into two?

FilipChalupa · 2019-06-12T08:17:46Z

Commit Failing build hotfix has been dropped. Cc. @enzy

Waiting for #39 to be merged.

- faster, better caching

enzy · 2019-06-12T12:20:39Z

src/templates/mixins/shape.pug

+		use(xlink:href=`${assetPath}/sprites/shapes.svg#shape-${name}`)
+
+		//- IE fallback
+		use(xlink:href=`#shape-${name}`)


The polyfill code should handle this case (replace all occurences of en external resource to local one) and it should not be included in the HTML twice.

This way the code can be easily used in lazy loaded html content.

IE is very bad at handling high JavaScript load. Scanning a whole page can do unnecessary harm during the page load while other work needs to be done too.

@enzy What do you think? Is the double use justified?

FilipChalupa · 2019-06-13T10:45:33Z

@JirkaVebr Is the <link rel="preload" as="image" href="/assets/shapes.svg"> still relevant?

enzy reviewed Jun 10, 2019

View reviewed changes

JirkaVebr suggested changes Jun 11, 2019

View reviewed changes

src/scripts/components/ShapesIE.ts Outdated Show resolved Hide resolved

FilipChalupa force-pushed the pr/shapes/faster branch from 0ce5a2b to 3e303cb Compare June 11, 2019 12:42

enzy suggested changes Jun 11, 2019

View reviewed changes

FilipChalupa force-pushed the pr/shapes/faster branch from 3e303cb to 7232d8d Compare June 12, 2019 08:09

FilipChalupa added 2 commits June 12, 2019 13:54

Shapes: js not needed

f0db558

- faster, better caching

Shapes: hasFeature call not needed

91de88c

FilipChalupa force-pushed the pr/shapes/faster branch from 7232d8d to 91de88c Compare June 12, 2019 11:55

FilipChalupa requested review from JirkaVebr and enzy and removed request for enzy June 12, 2019 11:56

enzy suggested changes Jun 12, 2019

View reviewed changes

Shapes: js not needed #38

Are you sure you want to change the base?

Shapes: js not needed #38

Uh oh!

Conversation

FilipChalupa commented Jun 10, 2019

Uh oh!

enzy left a comment

Choose a reason for hiding this comment

Uh oh!

FilipChalupa commented Jun 10, 2019

Uh oh!

JirkaVebr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

FilipChalupa commented Jun 11, 2019

Uh oh!

JirkaVebr commented Jun 11, 2019

Uh oh!

FilipChalupa commented Jun 11, 2019

Uh oh!

enzy commented Jun 11, 2019

Uh oh!

enzy left a comment

Choose a reason for hiding this comment

Uh oh!

FilipChalupa commented Jun 11, 2019

Uh oh!

FilipChalupa commented Jun 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

enzy commented Jun 11, 2019

Uh oh!

enzy commented Jun 11, 2019

Uh oh!

FilipChalupa commented Jun 11, 2019

Uh oh!

FilipChalupa commented Jun 11, 2019

Uh oh!

FilipChalupa commented Jun 11, 2019

Uh oh!

FilipChalupa commented Jun 12, 2019

Uh oh!

enzy Jun 12, 2019

Choose a reason for hiding this comment

Uh oh!

FilipChalupa Jun 12, 2019

Choose a reason for hiding this comment

Uh oh!

FilipChalupa Jun 13, 2019

Choose a reason for hiding this comment

Uh oh!

FilipChalupa commented Jun 13, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

FilipChalupa commented Jun 11, 2019 •

edited

Loading