[EuiIcon] Convert generated icon `.js` assets to `.tsx` files #5212

cee-chen · 2021-09-23T19:27:49Z

Summary

A nice capstone to my #5044 work - with this PR, all .js files in src/ are officially gone! 💥

I recommend following along by commit, and skimming commits that indicate they're primarily running yarn compile-icons 😅 I also left some code comments walking through specific changes, w/ documentation links.

QA

Confirm https://eui.elastic.co/pr_5212/#/display/icons all work as expected

Checklist

Check against all themes for compatibility in both light and dark modes

- [ ] Checked in mobile

Checked in Chrome, Safari, Edge, and Firefox

- [ ] Props have proper autodocs and playground toggles
~- [ ] Added documentation
~~- [ ] Checked Code Sandbox works for any docs examples~~

Added or updated jest tests

~~- [ ] Checked for breaking changes and labeled appropriately~~

Checked for accessibility including keyboard-only and screenreader modes
A changelog entry exists and is marked appropriately

- to get the latest features and perf improvements

- xmlns attr location changed for some reason :| - looks like it added xlink

- to match fs.readFileSync and .writeFileSync

- now that the files are .tsx, it's linting for the license NOTE: I could have added a eslint --fix command to yarn compile-icons, but doing so added another 30-40s of runtime to the command. Doing it in the writeFileSync keeps the command around 5-6s instead.

- by leaving a hint as to what file generator name to open/look for - now that these are actual .tsx files, it's going to be even less obvious to not edit them directly, and it's a nice developer affordance to say which file to look at rather than having to grep through the repo

yay, it works

package.json

cee-chen · 2021-09-23T19:42:09Z

scripts/compile-icons.js

@@ -27,7 +30,7 @@ iconFiles.forEach(async (filePath) => {

    const hasIds = svgString.includes('id="');

-    let jsxSource = await svgr(
+    let jsxSource = svgr.sync(


Optional refactor, I just saw the documentation for it in https://react-svgr.com/docs/node-api/#usage and thought .sync matched fs.readFileSync and fs.writeFileSync better

cee-chen · 2021-09-23T19:42:22Z

scripts/compile-icons.js

@@ -42,15 +45,17 @@ iconFiles.forEach(async (filePath) => {
          xmlns: 'http://www.w3.org/2000/svg',
        },
        titleProp: true,
+        typescript: true,


Documentation: https://react-svgr.com/docs/options/#typescript

scripts/compile-icons.js

cee-chen · 2021-09-23T19:43:48Z

scripts/compile-icons.js

-    const comment = '// THIS IS A GENERATED FILE. DO NOT MODIFY MANUALLY\n\n';
-    fs.writeFileSync(outputFilePath, comment + jsxSource);
+    const outputFilePath = filePath.replace(/\.svg$/, '.tsx');
+    const comment = `\n// THIS IS A GENERATED FILE. DO NOT MODIFY MANUALLY. @see scripts/compile-icons.js\n\n`;


Optional generated comment enhancement: now that the icon files are .tsx, it's even less obvious that you shouldn't edit them, so I figured it's nice to provide a @see link and filename for devs to jump to directly rather than having to grep through the repo

scripts/compile-icons.js

cee-chen · 2021-09-23T19:48:22Z

src/components/icon/__snapshots__/icon.test.tsx.snap

@@ -730,7 +730,7 @@ exports[`EuiIcon props type arrowDown is rendered 1`] = `
 >
  <path
    d="M13.069 5.157L8.384 9.768a.546.546 0 01-.768 0L2.93 5.158a.552.552 0 00-.771 0 .53.53 0 000 .759l4.684 4.61c.641.631 1.672.63 2.312 0l4.684-4.61a.53.53 0 000-.76.552.552 0 00-.771 0z"
-    fill-rule="non-zero"
+    fill-rule="nonzero"


Typescript caught this attribute issue, proving it works 🎉

kibanamachine · 2021-09-23T19:54:23Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_5212/

thompsongl · 2021-09-28T20:02:59Z

I tested this out in a simple create-react-app project, and icons fail to load:

And thinking about this all again, that makes sense. The icon files will get compiled to js during the build, so the tsx lookup doesn't find the file.
It'll work in the docs because of how webpack is used (I'm guessing the dist/ bundles work also), but not in the standard babel builds (lib/ and es/)

cee-chen · 2021-09-28T21:24:26Z

Ahhh 🤦‍♀️ Thanks for catching that Greg! I wonder if there's a way to detect "if compiled, looking for .js, if not, look for .tsx"... or falling back to .js if the .tsx file doesn't exist?

cee-chen · 2021-09-28T21:28:15Z

Or alternatively - is there a strong reason to import/load icons dynamically in the first place? Why not import them statically first and then find the right one via name/map later? 🤷

IIRC, tree-shaking isn't yet working in EUI/webpack so it's not really for perf reasons there, right?

thompsongl · 2021-09-29T16:19:40Z

A build-aware or try/catch solution sounds better, if possible. Even though treeshaking doesn't currently work, it's something we want eventually.

- webpack should be smart enough to attempt to load either a .tsx or a .js file automatically

cee-chen · 2021-09-29T17:16:19Z

@thompsongl So, earlier this morning I went down a 30min+ rabbit hole of seeing if I could convert the dynamic imports into static, and when I finally had it working and was doing some research on the perf implications, I had this epiphany...

"Why are we even specifying either .js or .tsx?" 🤦‍♀️

I'm 90% sure Webpack is typically smart enough to automatically import either TSX or JS modules over an .svg file (source, particularly the last 2 paragraphs). Removing the file extension completely works for me on local docs, and it also appears to be working for me in a local CRA app (although my setup could be off here). Worst comes to worst if for some reason it's struggling with the same file name existing as both .svg or .js/tsx, I can just split up the source SVGs into a different folder than the output .tsx files.

Let me know if the latest commit works for you! I tested on local Kibana dev as well and it appears to be working there 🤞

kibanamachine · 2021-09-29T17:38:36Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_5212/

kibanamachine · 2021-09-29T18:16:20Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_5212/

thompsongl · 2021-09-29T21:53:51Z

Lots of warnings in the build log related to .svg lookups. Webpack might be trying to load both files?

13:21:45 WARNING in ./components/icon/assets/tokens/tokenCompletionSuggester.svg 1:0
13:21:45 Module parse failed: Unexpected token (1:0)
13:21:45 You may need an appropriate loader to handle this file type, currently no loaders are configured to process this file. See https://webpack.js.org/concepts#loaders
13:21:45 >
13:21:45 |
13:21:45 |
13:21:45 @ ./components/icon/assets lazy ^./.*$ namespace object ./tokens/tokenCompletionSuggester.svg
13:21:45 @ ./components/icon/icon.tsx
13:21:45 @ ./components/icon/index.ts
13:21:45 @ ./components/index.ts
13:21:45 @ ./index.ts

cee-chen · 2021-09-30T15:46:41Z

Ahh gotcha, yeah I saw those on build as well but I actually wasn't sure if they'd always been there, and the built version appeared to work. I'll move the .tsx files into a separate folder to test that

- so webpack doesn't get confused when trying to dynamically import an extensionless file that has both a .tsx and .svg folder in the same dir - also allows us to flatten the tokens/ folder

- was doing a grep for /assets/ and got a little confused by the location of this

- moved faceNeutral (used) to face_neutral (unused) to match other casing

cee-chen · 2021-09-30T17:04:35Z

Not seeing the warnings anymore on build after splitting out the svg and generated tsx files into different folders! 🎉

kibanamachine · 2021-09-30T17:29:58Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_5212/

cee-chen · 2021-09-30T17:30:04Z

... oh wait, I forgot we tell people to access icon/assets directly in our wiki 🤦‍♀️ Kibana start is failing because of that in a security server-side page.

Going to push up a folder name change where I leave the generated files in assets but move the svgs to a new folder called svgs

- we can lint if we want to! we can leave your gitignore behind

cchaos · 2021-09-30T17:57:38Z

Can you also then update this portion of the wiki https://github.com/elastic/eui/blob/master/wiki/creating-icons.md#prepare-the-pull-request? 😄

cee-chen · 2021-09-30T18:00:07Z

Yeah! I should've just done that here: 5ab0988#diff-54bd9836a1fd4490b5678c576189707530d0b5fc051feb9cecc09c42a3595848

cee-chen · 2021-09-30T18:07:08Z

As of latest commit, EUI's yarn build-pack has no more warnings, Kibana start is working, and my test CRA app is also working 🧘

kibanamachine · 2021-09-30T18:26:10Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_5212/

thompsongl · 2021-10-01T21:17:53Z

This is looking good now!
Just want to see if @chandlerprall can think of any problems with removing the file extension from the dynamic import statement 🧐

chandlerprall · 2021-10-04T21:38:17Z

IIRC, tree-shaking isn't yet working in EUI/webpack so it's not really for perf reasons there, right?

Tree-shaking EUI does work (last I checked at least), but it requires enabling the production environment and not accidentally disabling it in the various ways that one could.

chandlerprall · 2021-10-04T21:41:55Z

Or alternatively - is there a strong reason to import/load icons dynamically in the first place? Why not import them statically first and then find the right one via name/map later? 🤷

We use the dynamic import to avoid including the icons in the core bundle, as it's unlikely most applications would use them all. Even in Kibana they are not all present on one page, and we see the benefits of not downloading all the svgs at once, which would otherwise add to the initial bundle download & parse times.

cee-chen · 2021-10-04T22:02:45Z

Super helpful to get that context - thanks Chandler! I was thankfully able to avoid changing the icon component from using dynamic import in the final solution.

chandlerprall

Changes LGTM! Extensionless import is fine, and I don't believe would have any effects.

kibanamachine · 2021-10-04T22:36:47Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_5212/

cee-chen added 11 commits September 23, 2021 10:48

Update svgr to 5.5.0

9b8ab0a

- to get the latest features and perf improvements

Rerun yarn compile-icons and update snapshots

3ac8e35

- xmlns attr location changed for some reason :| - looks like it added xlink

[Misc] Prefer svgr.sync API over await/async

6223c09

- to match fs.readFileSync and .writeFileSync

Update icons .js files to .tsx

db5f42d

Fix import ts-ignores and types

94827df

Fix missing SVGRProps definition

493b73c

Fix missing license lint

abe5551

- now that the files are .tsx, it's linting for the license NOTE: I could have added a eslint --fix command to yarn compile-icons, but doing so added another 30-40s of runtime to the command. Doing it in the writeFileSync keeps the command around 5-6s instead.

Remove all icon .js assets and run yarn compile-icons

184bc3f

Fix SVG attribute error (caught by Typescript!)

40653dd

yay, it works

Manually convert last SVG icon .js files used by EuiMarkdownEditor

7634e7f

cee-chen commented Sep 23, 2021

View reviewed changes

cee-chen marked this pull request as ready for review September 23, 2021 19:50

cee-chen added tech debt skip-changelog labels Sep 23, 2021

Merge branch 'master' into compile-icons-tsx

5599d43

Remove .tsx extension from dynamic import

177df3e

- webpack should be smart enough to attempt to load either a .tsx or a .js file automatically

Add changelog entry

deba0cd

cee-chen removed the skip-changelog label Sep 29, 2021

Merge branch 'master' into compile-icons-tsx

b363e51

cee-chen added 3 commits September 30, 2021 09:41

Move generated .tsx files to separate folder

700eb06

- so webpack doesn't get confused when trying to dynamically import an extensionless file that has both a .tsx and .svg folder in the same dir - also allows us to flatten the tokens/ folder

[misc] clean up compile-eui script

b8a8072

- was doing a grep for /assets/ and got a little confused by the location of this

Fix duplicate face neutral icon

4c577ed

- moved faceNeutral (used) to face_neutral (unused) to match other casing

cee-chen added 2 commits September 30, 2021 10:51

Leave generated icons in assets/, move svgs to different folder

5ab0988

Remove now-unused eslintignore

112ae6d

- we can lint if we want to! we can leave your gitignore behind

chandlerprall approved these changes Oct 4, 2021

View reviewed changes

Merge branch 'master' into compile-icons-tsx

cae465a

cee-chen enabled auto-merge (squash) October 4, 2021 22:11

cee-chen merged commit 2aa5bb0 into elastic:master Oct 4, 2021

cee-chen deleted the compile-icons-tsx branch October 4, 2021 22:47

breehall mentioned this pull request Oct 6, 2021

Bumping EUI to 39.0.0 elastic/kibana#113633

Merged

cee-chen mentioned this pull request Oct 21, 2021

Error: Cannot find module './assets/lock' when using <EuiFieldPassword /> component #5286

Closed

gbamparop mentioned this pull request Nov 2, 2021

[wiki] Filenames still have .js extension on the wiki #5347

Closed

cee-chen mentioned this pull request Dec 14, 2021

Vite/Rollup/ESBuild cannot process the dynamic imports in EuiIcon #5463

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[EuiIcon] Convert generated icon `.js` assets to `.tsx` files #5212

[EuiIcon] Convert generated icon `.js` assets to `.tsx` files #5212

cee-chen commented Sep 23, 2021 •

edited

Loading

cee-chen Sep 23, 2021

cee-chen Sep 23, 2021

cee-chen Sep 23, 2021

cee-chen Sep 23, 2021

kibanamachine commented Sep 23, 2021

thompsongl commented Sep 28, 2021

cee-chen commented Sep 28, 2021

cee-chen commented Sep 28, 2021

thompsongl commented Sep 29, 2021

cee-chen commented Sep 29, 2021 •

edited

Loading

kibanamachine commented Sep 29, 2021

kibanamachine commented Sep 29, 2021

thompsongl commented Sep 29, 2021

cee-chen commented Sep 30, 2021

cee-chen commented Sep 30, 2021

kibanamachine commented Sep 30, 2021

cee-chen commented Sep 30, 2021 •

edited

Loading

cchaos commented Sep 30, 2021

cee-chen commented Sep 30, 2021

cee-chen commented Sep 30, 2021

kibanamachine commented Sep 30, 2021

thompsongl commented Oct 1, 2021

chandlerprall commented Oct 4, 2021

chandlerprall commented Oct 4, 2021

cee-chen commented Oct 4, 2021

chandlerprall left a comment

kibanamachine commented Oct 4, 2021

[EuiIcon] Convert generated icon .js assets to .tsx files #5212

[EuiIcon] Convert generated icon .js assets to .tsx files #5212

Conversation

cee-chen commented Sep 23, 2021 • edited Loading

Summary

QA

Checklist

cee-chen Sep 23, 2021

Choose a reason for hiding this comment

cee-chen Sep 23, 2021

Choose a reason for hiding this comment

cee-chen Sep 23, 2021

Choose a reason for hiding this comment

cee-chen Sep 23, 2021

Choose a reason for hiding this comment

kibanamachine commented Sep 23, 2021

thompsongl commented Sep 28, 2021

cee-chen commented Sep 28, 2021

cee-chen commented Sep 28, 2021

thompsongl commented Sep 29, 2021

cee-chen commented Sep 29, 2021 • edited Loading

kibanamachine commented Sep 29, 2021

kibanamachine commented Sep 29, 2021

thompsongl commented Sep 29, 2021

cee-chen commented Sep 30, 2021

cee-chen commented Sep 30, 2021

kibanamachine commented Sep 30, 2021

cee-chen commented Sep 30, 2021 • edited Loading

cchaos commented Sep 30, 2021

cee-chen commented Sep 30, 2021

cee-chen commented Sep 30, 2021

kibanamachine commented Sep 30, 2021

thompsongl commented Oct 1, 2021

chandlerprall commented Oct 4, 2021

chandlerprall commented Oct 4, 2021

cee-chen commented Oct 4, 2021

chandlerprall left a comment

Choose a reason for hiding this comment

kibanamachine commented Oct 4, 2021

[EuiIcon] Convert generated icon `.js` assets to `.tsx` files #5212

[EuiIcon] Convert generated icon `.js` assets to `.tsx` files #5212

cee-chen commented Sep 23, 2021 •

edited

Loading

cee-chen commented Sep 29, 2021 •

edited

Loading

cee-chen commented Sep 30, 2021 •

edited

Loading