From c62d3353a2a2a69d334e4bc448a5cd3c3d99f55a Mon Sep 17 00:00:00 2001 From: Mikhail Bashkirov Date: Thu, 27 Jun 2019 15:15:49 +0200 Subject: [PATCH 1/7] fix(icon): render nothing consistently when svg is undefined --- packages/icon/src/LionIcon.js | 2 +- packages/icon/test/lion-icon.test.js | 12 ++++++++++++ 2 files changed, 13 insertions(+), 1 deletion(-) diff --git a/packages/icon/src/LionIcon.js b/packages/icon/src/LionIcon.js index 8934b907a..a6b4fea4f 100644 --- a/packages/icon/src/LionIcon.js +++ b/packages/icon/src/LionIcon.js @@ -89,7 +89,7 @@ export class LionIcon extends LionLitElement { * so make sure to have in svg files */ _setSvg() { - this.innerHTML = this.svg; + this.innerHTML = this.svg ? this.svg : ''; } // TODO: find a better way to render dynamic icons without the need for unsafeHTML diff --git a/packages/icon/test/lion-icon.test.js b/packages/icon/test/lion-icon.test.js index 950766cdf..beb019692 100644 --- a/packages/icon/test/lion-icon.test.js +++ b/packages/icon/test/lion-icon.test.js @@ -149,4 +149,16 @@ describe('lion-icon', () => { expect(el.children[0].id).to.equal('svg-heart'); }); + + it('does not render "undefined" if changed from valid input to undefined', async () => { + const el = await fixture( + html` + + `, + ); + await el.updateComplete; + el.svg = undefined; + await el.updateComplete; + expect(el.innerHTML).to.equal(''); + }); }); From bd5f51e3d7c3703a83dfec19b61f7fcdb7649f62 Mon Sep 17 00:00:00 2001 From: Mikhail Bashkirov Date: Thu, 27 Jun 2019 16:10:49 +0200 Subject: [PATCH 2/7] fix(icon): use LitElement --- packages/icon/src/LionIcon.js | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/packages/icon/src/LionIcon.js b/packages/icon/src/LionIcon.js index a6b4fea4f..ea5cbf424 100644 --- a/packages/icon/src/LionIcon.js +++ b/packages/icon/src/LionIcon.js @@ -1,5 +1,4 @@ -import { html, css, render, unsafeHTML, until } from '@lion/core'; -import { LionLitElement } from '@lion/core/src/LionLitElement.js'; +import { html, css, render, unsafeHTML, until, LitElement } from '@lion/core'; const isDefinedPromise = action => typeof action === 'object' && Promise.resolve(action) === action; @@ -7,7 +6,7 @@ const isDefinedPromise = action => typeof action === 'object' && Promise.resolve * Custom element for rendering SVG icons * @polymerElement */ -export class LionIcon extends LionLitElement { +export class LionIcon extends LitElement { static get properties() { return { svg: { From ba69c52ff1d9ca8dcec214d214ebd3d5418a9653 Mon Sep 17 00:00:00 2001 From: Mikhail Bashkirov Date: Thu, 27 Jun 2019 17:25:23 +0200 Subject: [PATCH 3/7] chore(icon): add tests for race conditions with promises --- packages/icon/test/lion-icon.test.js | 85 +++++++++++++++++++++++++++- 1 file changed, 83 insertions(+), 2 deletions(-) diff --git a/packages/icon/test/lion-icon.test.js b/packages/icon/test/lion-icon.test.js index beb019692..0587fe598 100644 --- a/packages/icon/test/lion-icon.test.js +++ b/packages/icon/test/lion-icon.test.js @@ -1,5 +1,5 @@ -import { expect, fixture, aTimeout, html } from '@open-wc/testing'; -import { until } from '@lion/core'; +import { expect, fixture, fixtureSync, aTimeout, html } from '@open-wc/testing'; +import { until, render } from '@lion/core'; import heartSvg from './heart.svg.js'; import hammerSvg from './hammer.svg.js'; @@ -161,4 +161,85 @@ describe('lion-icon', () => { await el.updateComplete; expect(el.innerHTML).to.equal(''); }); + + describe('race conditions with dynamic promisified icons', () => { + async function prepareRaceCondition(...svgs) { + const container = fixtureSync(`
`); + const resolves = svgs.map(svg => { + let resolveSvg; + + const svgProperty = + Promise.resolve(svg) === svg + ? new Promise(resolve => { + resolveSvg = () => resolve(svg); + }) + : svg; + + render( + html` + + `, + container, + ); + + return resolveSvg; + }); + + const icon = container.children[0]; + await icon.updateComplete; + return [icon, ...resolves]; + } + + it('renders in the order of rendering instead of the order of resolution', async () => { + let resolveHeartSvg; + let resolveHammerSvg; + let icon; + let svg; + + [icon, resolveHeartSvg, resolveHammerSvg] = await prepareRaceCondition( + Promise.resolve(heartSvg), + Promise.resolve(hammerSvg), + ); + resolveHeartSvg(); + resolveHammerSvg(); + await aTimeout(); + [svg] = icon.children; + expect(svg).to.exist; + expect(svg.id).to.equal('svg-hammer'); + + [icon, resolveHeartSvg, resolveHammerSvg] = await prepareRaceCondition( + Promise.resolve(heartSvg), + Promise.resolve(hammerSvg), + ); + resolveHammerSvg(); + resolveHeartSvg(); + await aTimeout(); + [svg] = icon.children; + expect(svg).to.exist; + expect(svg.id).to.equal('svg-hammer'); + }); + + it('renders if a resolved promise was replaced by a string', async () => { + const [icon, resolveHeartSvg] = await prepareRaceCondition( + Promise.resolve(heartSvg), + hammerSvg, + ); + resolveHeartSvg(); + await aTimeout(); + const [svg] = icon.children; + expect(svg).to.exist; + expect(svg.id).to.equal('svg-hammer'); + }); + + it('does not render if a resolved promise was replaced by another unresolved promise', async () => { + const [icon, resolveHeartSvg] = await prepareRaceCondition( + Promise.resolve(heartSvg), + Promise.resolve(hammerSvg), + ); + resolveHeartSvg(); + await aTimeout(); + const [svg] = icon.children; + expect(svg).to.not.exist; + }); + }); }); From d5bb4f5a2bc8f7ca0ca8520027ebd75bc0fc6a29 Mon Sep 17 00:00:00 2001 From: Mikhail Bashkirov Date: Thu, 27 Jun 2019 17:20:42 +0200 Subject: [PATCH 4/7] chore: remove unused assets --- assets/icons/bugs/bug05.svg.js | 1 - assets/icons/bugs/bug12.svg.js | 1 - 2 files changed, 2 deletions(-) delete mode 100644 assets/icons/bugs/bug05.svg.js delete mode 100644 assets/icons/bugs/bug12.svg.js diff --git a/assets/icons/bugs/bug05.svg.js b/assets/icons/bugs/bug05.svg.js deleted file mode 100644 index 48eaacfed..000000000 --- a/assets/icons/bugs/bug05.svg.js +++ /dev/null @@ -1 +0,0 @@ -export default ''; diff --git a/assets/icons/bugs/bug12.svg.js b/assets/icons/bugs/bug12.svg.js deleted file mode 100644 index 6c1ff569f..000000000 --- a/assets/icons/bugs/bug12.svg.js +++ /dev/null @@ -1 +0,0 @@ -export default ''; From 099f2d7c9df6d8ec2ec438e7eb978ccb12735bf8 Mon Sep 17 00:00:00 2001 From: Mikhail Bashkirov Date: Thu, 27 Jun 2019 17:55:32 +0200 Subject: [PATCH 5/7] chore(icon): fix SVGs to prevent focus in IE11 --- packages/icon/stories/icons/bugs/bug01.svg.js | 2 +- packages/icon/stories/icons/bugs/bug02.svg.js | 2 +- packages/icon/stories/icons/bugs/bug05.svg.js | 2 +- packages/icon/stories/icons/bugs/bug06.svg.js | 2 +- packages/icon/stories/icons/bugs/bug08.svg.js | 2 +- packages/icon/stories/icons/bugs/bug12.svg.js | 2 +- packages/icon/stories/icons/bugs/bug19.svg.js | 2 +- packages/icon/stories/icons/bugs/bug23.svg.js | 2 +- packages/icon/stories/icons/bugs/bug24.svg.js | 2 +- packages/icon/stories/icons/space/aliens-spaceship.svg.js | 2 +- packages/icon/stories/icons/space/meteor.svg.js | 2 +- packages/icon/stories/icons/space/moon-flag.svg.js | 2 +- packages/icon/stories/icons/space/moon.svg.js | 2 +- packages/icon/stories/icons/space/night.svg.js | 2 +- packages/icon/stories/icons/space/orbit.svg.js | 2 +- packages/icon/stories/icons/space/planet.svg.js | 2 +- packages/icon/stories/icons/space/robot.svg.js | 2 +- packages/icon/stories/icons/space/rocket.svg.js | 2 +- packages/icon/stories/icons/space/satellite.svg.js | 2 +- packages/icon/stories/icons/space/signal.svg.js | 2 +- packages/icon/stories/icons/space/space-helmet.svg.js | 2 +- packages/icon/stories/icons/space/sun.svg.js | 2 +- packages/icon/stories/icons/space/telescope.svg.js | 2 +- packages/icon/test/heart.svg.js | 2 +- 24 files changed, 24 insertions(+), 24 deletions(-) diff --git a/packages/icon/stories/icons/bugs/bug01.svg.js b/packages/icon/stories/icons/bugs/bug01.svg.js index 029fd461d..c57d889b2 100644 --- a/packages/icon/stories/icons/bugs/bug01.svg.js +++ b/packages/icon/stories/icons/bugs/bug01.svg.js @@ -1 +1 @@ -export default ''; +export default ''; diff --git a/packages/icon/stories/icons/bugs/bug02.svg.js b/packages/icon/stories/icons/bugs/bug02.svg.js index 5d33ebc1c..5211c1e8f 100644 --- a/packages/icon/stories/icons/bugs/bug02.svg.js +++ b/packages/icon/stories/icons/bugs/bug02.svg.js @@ -1 +1 @@ -export default ''; +export default ''; diff --git a/packages/icon/stories/icons/bugs/bug05.svg.js b/packages/icon/stories/icons/bugs/bug05.svg.js index 48eaacfed..33fe059b4 100644 --- a/packages/icon/stories/icons/bugs/bug05.svg.js +++ b/packages/icon/stories/icons/bugs/bug05.svg.js @@ -1 +1 @@ -export default ''; +export default ''; diff --git a/packages/icon/stories/icons/bugs/bug06.svg.js b/packages/icon/stories/icons/bugs/bug06.svg.js index 04d5fdd1d..8d5caf285 100644 --- a/packages/icon/stories/icons/bugs/bug06.svg.js +++ b/packages/icon/stories/icons/bugs/bug06.svg.js @@ -1 +1 @@ -export default ''; +export default ''; diff --git a/packages/icon/stories/icons/bugs/bug08.svg.js b/packages/icon/stories/icons/bugs/bug08.svg.js index a0f6c1115..d2c7c504e 100644 --- a/packages/icon/stories/icons/bugs/bug08.svg.js +++ b/packages/icon/stories/icons/bugs/bug08.svg.js @@ -1 +1 @@ -export default ''; +export default ''; diff --git a/packages/icon/stories/icons/bugs/bug12.svg.js b/packages/icon/stories/icons/bugs/bug12.svg.js index 6c1ff569f..9a42c96c2 100644 --- a/packages/icon/stories/icons/bugs/bug12.svg.js +++ b/packages/icon/stories/icons/bugs/bug12.svg.js @@ -1 +1 @@ -export default ''; +export default ''; diff --git a/packages/icon/stories/icons/bugs/bug19.svg.js b/packages/icon/stories/icons/bugs/bug19.svg.js index 4f29b31bd..832aebafa 100644 --- a/packages/icon/stories/icons/bugs/bug19.svg.js +++ b/packages/icon/stories/icons/bugs/bug19.svg.js @@ -1 +1 @@ -export default ''; +export default ''; diff --git a/packages/icon/stories/icons/bugs/bug23.svg.js b/packages/icon/stories/icons/bugs/bug23.svg.js index d7e4842ec..330716eaa 100644 --- a/packages/icon/stories/icons/bugs/bug23.svg.js +++ b/packages/icon/stories/icons/bugs/bug23.svg.js @@ -1 +1 @@ -export default ''; +export default ''; diff --git a/packages/icon/stories/icons/bugs/bug24.svg.js b/packages/icon/stories/icons/bugs/bug24.svg.js index e6d05603b..7e77f6b9b 100644 --- a/packages/icon/stories/icons/bugs/bug24.svg.js +++ b/packages/icon/stories/icons/bugs/bug24.svg.js @@ -1 +1 @@ -export default ''; +export default ''; diff --git a/packages/icon/stories/icons/space/aliens-spaceship.svg.js b/packages/icon/stories/icons/space/aliens-spaceship.svg.js index a2e6c380b..a6cd30a53 100644 --- a/packages/icon/stories/icons/space/aliens-spaceship.svg.js +++ b/packages/icon/stories/icons/space/aliens-spaceship.svg.js @@ -1 +1 @@ -export default ''; +export default ''; diff --git a/packages/icon/stories/icons/space/meteor.svg.js b/packages/icon/stories/icons/space/meteor.svg.js index f3b91df87..40770e093 100644 --- a/packages/icon/stories/icons/space/meteor.svg.js +++ b/packages/icon/stories/icons/space/meteor.svg.js @@ -1 +1 @@ -export default ''; +export default ''; diff --git a/packages/icon/stories/icons/space/moon-flag.svg.js b/packages/icon/stories/icons/space/moon-flag.svg.js index f34cd3a42..2be77c6b9 100644 --- a/packages/icon/stories/icons/space/moon-flag.svg.js +++ b/packages/icon/stories/icons/space/moon-flag.svg.js @@ -1 +1 @@ -export default ''; +export default ''; diff --git a/packages/icon/stories/icons/space/moon.svg.js b/packages/icon/stories/icons/space/moon.svg.js index 726e67d72..ac5cf1c1d 100644 --- a/packages/icon/stories/icons/space/moon.svg.js +++ b/packages/icon/stories/icons/space/moon.svg.js @@ -1 +1 @@ -export default ''; +export default ''; diff --git a/packages/icon/stories/icons/space/night.svg.js b/packages/icon/stories/icons/space/night.svg.js index 3d2f92dc4..8528f3dc2 100644 --- a/packages/icon/stories/icons/space/night.svg.js +++ b/packages/icon/stories/icons/space/night.svg.js @@ -1 +1 @@ -export default ''; +export default ''; diff --git a/packages/icon/stories/icons/space/orbit.svg.js b/packages/icon/stories/icons/space/orbit.svg.js index 468495378..9819d763f 100644 --- a/packages/icon/stories/icons/space/orbit.svg.js +++ b/packages/icon/stories/icons/space/orbit.svg.js @@ -1 +1 @@ -export default ''; +export default ''; diff --git a/packages/icon/stories/icons/space/planet.svg.js b/packages/icon/stories/icons/space/planet.svg.js index 986b0f369..2364e9af8 100644 --- a/packages/icon/stories/icons/space/planet.svg.js +++ b/packages/icon/stories/icons/space/planet.svg.js @@ -1 +1 @@ -export default ''; +export default ''; diff --git a/packages/icon/stories/icons/space/robot.svg.js b/packages/icon/stories/icons/space/robot.svg.js index d18488723..fa6609620 100644 --- a/packages/icon/stories/icons/space/robot.svg.js +++ b/packages/icon/stories/icons/space/robot.svg.js @@ -1 +1 @@ -export default ''; +export default ''; diff --git a/packages/icon/stories/icons/space/rocket.svg.js b/packages/icon/stories/icons/space/rocket.svg.js index b0b036e73..68dae39d7 100644 --- a/packages/icon/stories/icons/space/rocket.svg.js +++ b/packages/icon/stories/icons/space/rocket.svg.js @@ -1 +1 @@ -export default ''; +export default ''; diff --git a/packages/icon/stories/icons/space/satellite.svg.js b/packages/icon/stories/icons/space/satellite.svg.js index b44c59c49..0291c2f02 100644 --- a/packages/icon/stories/icons/space/satellite.svg.js +++ b/packages/icon/stories/icons/space/satellite.svg.js @@ -1 +1 @@ -export default ''; +export default ''; diff --git a/packages/icon/stories/icons/space/signal.svg.js b/packages/icon/stories/icons/space/signal.svg.js index fca4008f9..e7e1dbb6f 100644 --- a/packages/icon/stories/icons/space/signal.svg.js +++ b/packages/icon/stories/icons/space/signal.svg.js @@ -1 +1 @@ -export default ''; +export default ''; diff --git a/packages/icon/stories/icons/space/space-helmet.svg.js b/packages/icon/stories/icons/space/space-helmet.svg.js index eea6c5648..45300e92f 100644 --- a/packages/icon/stories/icons/space/space-helmet.svg.js +++ b/packages/icon/stories/icons/space/space-helmet.svg.js @@ -1 +1 @@ -export default ''; +export default ''; diff --git a/packages/icon/stories/icons/space/sun.svg.js b/packages/icon/stories/icons/space/sun.svg.js index fd7fa97e8..134f315a6 100644 --- a/packages/icon/stories/icons/space/sun.svg.js +++ b/packages/icon/stories/icons/space/sun.svg.js @@ -1 +1 @@ -export default ''; +export default ''; diff --git a/packages/icon/stories/icons/space/telescope.svg.js b/packages/icon/stories/icons/space/telescope.svg.js index baef9a146..498c6f8b6 100644 --- a/packages/icon/stories/icons/space/telescope.svg.js +++ b/packages/icon/stories/icons/space/telescope.svg.js @@ -1 +1 @@ -export default ''; +export default ''; diff --git a/packages/icon/test/heart.svg.js b/packages/icon/test/heart.svg.js index d0f635236..0617efdbb 100644 --- a/packages/icon/test/heart.svg.js +++ b/packages/icon/test/heart.svg.js @@ -1 +1 @@ -export default ''; +export default ''; From d1c11616e41eec6a6152cfc1b75a00187a7f7c29 Mon Sep 17 00:00:00 2001 From: Mikhail Bashkirov Date: Thu, 27 Jun 2019 18:02:24 +0200 Subject: [PATCH 6/7] chore(icon): clean up SVG bundle module for tests --- packages/icon/test/myIcon.bundle.js | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/packages/icon/test/myIcon.bundle.js b/packages/icon/test/myIcon.bundle.js index cde5b431f..82abcddd3 100644 --- a/packages/icon/test/myIcon.bundle.js +++ b/packages/icon/test/myIcon.bundle.js @@ -1,3 +1 @@ -export default ''; -export const heart = - ''; +export { default as heart } from './heart.svg.js'; From 675fc615ef51674c8c2999b39d28c1b0fcf085a8 Mon Sep 17 00:00:00 2001 From: Goffert van Gool Date: Thu, 27 Jun 2019 16:33:36 +0200 Subject: [PATCH 7/7] fix(icon): refactor icon to not use 'until' hack and use get/set --- packages/icon/src/LionIcon.js | 50 +++++++++++++++++------------------ 1 file changed, 25 insertions(+), 25 deletions(-) diff --git a/packages/icon/src/LionIcon.js b/packages/icon/src/LionIcon.js index ea5cbf424..1d492511a 100644 --- a/packages/icon/src/LionIcon.js +++ b/packages/icon/src/LionIcon.js @@ -1,6 +1,6 @@ -import { html, css, render, unsafeHTML, until, LitElement } from '@lion/core'; +import { html, css, LitElement } from '@lion/core'; -const isDefinedPromise = action => typeof action === 'object' && Promise.resolve(action) === action; +const isPromise = action => typeof action === 'object' && Promise.resolve(action) === action; /** * Custom element for rendering SVG icons @@ -9,6 +9,7 @@ const isDefinedPromise = action => typeof action === 'object' && Promise.resolve export class LionIcon extends LitElement { static get properties() { return { + // svg is a property to ensure the setter is called if the property is set before upgrading svg: { type: String, }, @@ -59,13 +60,6 @@ export class LionIcon extends LitElement { update(changedProperties) { super.update(changedProperties); - if (changedProperties.has('svg')) { - if (isDefinedPromise(this.svg)) { - this._setDynamicSvg(); - } else { - this._setSvg(); - } - } if (changedProperties.has('ariaLabel')) { this._onLabelChanged(changedProperties); } @@ -87,24 +81,25 @@ export class LionIcon extends LitElement { * On IE11, svgs without focusable false appear in the tab order * so make sure to have in svg files */ - _setSvg() { - this.innerHTML = this.svg ? this.svg : ''; + set svg(svg) { + this.__svg = svg; + if (svg === undefined) { + this._renderSvg(''); + } else if (isPromise(svg)) { + this._renderSvg(''); // show nothing before resolved + svg.then(resolvedSvg => { + // render only if it is still the same and was not replaced after loading started + if (svg === this.__svg) { + this._renderSvg(resolvedSvg); + } + }); + } else { + this._renderSvg(svg); + } } - // TODO: find a better way to render dynamic icons without the need for unsafeHTML - _setDynamicSvg() { - const template = html` - ${until( - this.svg.then(_svg => { - // If the export was not made explicit, take the default - if (typeof _svg !== 'string') { - return unsafeHTML(_svg.default); - } - return unsafeHTML(_svg); - }), - )} - `; - render(template, this); + get svg() { + return this.__svg; } _onLabelChanged() { @@ -115,4 +110,9 @@ export class LionIcon extends LitElement { this.removeAttribute('aria-label'); } } + + _renderSvg(svgOrModule) { + const svg = svgOrModule && svgOrModule.default ? svgOrModule.default : svgOrModule; + this.innerHTML = svg; + } }