[icons] Enable tree-shaking for SVG icons #2356

reiv · 2018-04-03T23:15:06Z

Fixes #2193

Changes proposed in this pull request:

This PR restructures the icons package to enable tree shaking for SVG icons. This should reduce the build size in production by quite a bit (around 350-400 KiB). Because the current icons architecture is incompatible with tree shaking, this optimization needs to be enabled with a compile flag, tentatively named BLUEPRINT_ICONS_TREE_SHAKING. Enabling this flag means that the icon prop of Icon components can no longer be specified as a string, and prevents the entirety of the icons package from being bundled by default. Instead, it falls to the consumer to manually import whichever icons are needed in that project. SVG icons are exported at the root level of the icons package, using aliases that are ~~camelCased~~ PascalCased versions of the icon names suffixed with "Icon". For example:

import { AlignLeftIcon, FunctionIcon } from "@blueprintjs/icons";

const icon = <FunctionIcon ... />;

Some Webpack build sizes:

Without tree shaking

  Asset     Size  Chunks                    Chunk Names
main.js  536 KiB       0  [emitted]  [big]  main

With tree shaking

  Asset     Size  Chunks             Chunk Names
main.js  138 KiB       0  [emitted]  main

The tree shaking flag can be set using webpack's DefinePlugin as follows:

new webpack.DefinePlugin({
    "global.BLUEPRINT_ICONS_TREE_SHAKING": true,
})

This will break everything that uses string-form icon props (nothing has been ported over yet).

reiv · 2018-04-03T23:22:23Z

Bail if icon name is invalid

Preview: documentation | landing | table

giladgray

🎆 @reiv this is some great stuff! i'm mildly uncomfortable with the SVGIcon type and the functionIcon naming scheme, but i see why you went that route. i wonder what it would take to have FunctionIcon be an actual JSX element?

giladgray · 2018-04-05T17:49:36Z

config/tsconfig.base.json

@@ -24,6 +24,7 @@
        "removeComments": false,
        "sourceMap": false,
        "stripInternal": true,
-        "target": "es5"
+        "target": "es5",
+        "typeRoots": ["../node_modules/@types"]


why is this necessary now? we've never needed it in the past.

This was just something I had to add on my end because of a bug with at-loader on Windows. In particular, it was causing Typescript to complain about being unable to find require(). There's some discussion of it here: PatrickJS/PatrickJS-starter#1371
It shouldn't affect anything else, but happy to split this off into a separate PR to reduce noise in the diff.

hmmm core declares the require() type. we should probably just do the same in icons rather than changing the tsconfig.

However, this also fixes a slew of errors that occur as a result of that at-loader + Windows interaction. For example, it doesn't pick up the typings for Mocha, so compiling unit tests gives me a wall of can't find "it/describe/etc" errors. I'll definitely submit this as a separate PR then.

giladgray · 2018-04-05T17:52:22Z

packages/node-build-scripts/generate-icons-source

        ICONS_METADATA.map(async icon => {
            const filepath = path.resolve(__dirname, `../../resources/icons/${size}px/${icon.iconName}.svg`);
            const svg = fs.readFileSync(filepath, "utf-8");
            const pathStrings = await svgo
                .optimize(svg, { path: filepath })
                .then(({ data }) => data.match(/ d="[^"]+"/g) || [])
                .then(paths => paths.map(s => s.slice(3)));
-            return `    "${icon.iconName}": [${pathStrings.join(",\n")}],`;
+            paths[icon.iconName] = `[${pathStrings.join(",\n")}]`;


this is now a reduce, not a map.

ICONS_METADATA.reduce(async (paths, icon) => { ... }, {})

giladgray · 2018-04-05T17:53:46Z

packages/node-build-scripts/generate-icons-source

+ */
+function buildPathsObject(paths) {
+    return Object.keys(paths).map(iconName => {
+        return `    "${iconName}": ${paths[iconName]},`


inline! iconName => "template-string"

reiv · 2018-04-05T19:13:27Z

@giladgray If ${name}Icon is too verbose, how about pt${name}?

i wonder what it would take to have FunctionIcon be an actual JSX element?

Open to exploring this option as well (I was thinking about it too). All it would take is exporting the icons as SFCs or something; basically a function that returns an <Icon> element with the icon pre-filled in. I am concerned about the overhead that would add, though. Also, we'd get a weird circular dependency between icons and core. Is there a reason <Icon> should be defined in core and not icons?

reiv · 2018-04-05T19:23:15Z

Refactor for code style

Preview: documentation | landing | table

giladgray · 2018-04-05T21:27:32Z

let's avoid pt-anything because we're actually going to change the prefix in 3.0 (hence #2325).

the main reason <Icon> doesn't live in the icons package is that it uses Intent in its props 😅

giladgray · 2018-04-05T21:28:24Z

packages/node-build-scripts/generate-icons-source

-        }),
-    );
+            paths[icon.iconName] = `[${pathStrings.join(",\n")}]`;
+  return paths;


something weird going on with whitespace here?

reiv · 2018-04-05T21:44:07Z

@giladgray it'll be hard to pursue the individual-icons-as-Components idea if <Icon> lives in core because of that circular dependency (TS imports are sequential AFAIK). Is there a workaround for that so we can reference <Icon> from within the icons package?

reiv · 2018-04-05T21:52:31Z

Fix whitespace

Preview: documentation | landing | table

This reverts commit c45e459.

reiv · 2018-04-06T20:33:36Z

A few more thoughts following up on the above:

In my opinion the most elegant solution would be to define <Icon> in the icons package (probably as it should have been were it not for its dependency on certain props and functions defined in core). The question is how to avoid the circular dependency so that only core is dependent on icons and not vice-versa. The following things from core are required by Icon:
- IProps and IIntentProps interfaces, and by extension, the Intent enum
- Classes.Icon string constant
- Classes.intentClass function
It's not much, so it could probably be duplicated in icons (types have no cost in compile size anyway). The alternative to duplication would be to define these in a separate commons package that's used by both core and icons, but I don't think that's a good idea, simply because it'd require consumers to import yet another package.
Another way would perhaps be to embrace the circular dependency and make it possible to import stuff from core in icons. Currently this is failing because the d.ts declarations in core/lib import types from "@blueprintjs/icons", which resolves to icons/lib. At this point in compilation, icons/lib either doesn't exist yet, or it does. Both are a problem. If it doesn't exist, tsc will abort with Cannot find module '@blueprintjs/icons'. If it does, tsc will complain about overwriting the declarations in icons/lib because those are now treated as "inputs" due to being referenced in core (AFAIK there is no compiler flag to force overwriting). Either way, I suppose we would need a way to "bootstrap" the icons package so that core can import the needed types in a way that doesn't interfere with actually compiling the icons package.

I'll probably try the former first and see where it takes us (can always revert if it's a no-go.)

giladgray

looking good! i'm gonna have to check this out locally and play with it before final sign-off.

giladgray · 2018-04-16T23:22:56Z

packages/icons/src/common/props.ts

@@ -0,0 +1,71 @@
+/*
+ * Copyright 2015 Palantir Technologies, Inc. All rights reserved.


please update all copyrights to 2018

giladgray · 2018-04-16T23:24:33Z

packages/node-build-scripts/generate-icons-source

+            .then(({ data }) => data.match(/ d="[^"]+"/g) || [])
+            .then(paths => paths.map(s => s.slice(3)));
+        paths[icon.iconName] = `[${pathStrings.join(",\n")}]`;
+  return paths;


indentation?

giladgray · 2018-04-16T23:26:07Z

packages/icons/src/components/icon/icon.tsx

+    public static displayName = "Blueprint2.Icon";
+
+    public static readonly SIZE_STANDARD = 16;
+    public static readonly SIZE_LARGE = 20;


reference the SVGIcon constants instead of redeclaring constants

giladgray · 2018-04-16T23:28:06Z

packages/core/src/common/errors.ts

@@ -26,6 +26,11 @@ export const HOTKEYS_WARN_DECORATOR_NO_METHOD = ns + ` @HotkeysTarget-decorated
 export const HOTKEYS_WARN_DECORATOR_NEEDS_REACT_ELEMENT =
    ns + ` "@HotkeysTarget-decorated components must return a single JSX.Element or an empty render.`;

+export const ICON_STRING_NAMES_NOT_SUPPORTED =


is this used in core package? pretty sure it's safe to delete as it's defined in icons below.

giladgray · 2018-04-16T23:30:50Z

packages/icons/src/components/icon/icon.tsx

+
+let allIcons: { [key: string]: IconPartial | undefined } | null = null;
+
+if (!(global as any).BLUEPRINT_ICONS_TREE_SHAKING) {


🤔 if a user never imports this component and only uses <NamedIcon /> exports, then will this file (and allIcons) be tree-shook automatically? as in, do we actually need this flag or can we rely on the build tool to detect that this module is unused?

But Icon is still used under the hood in other components, so it's unlikely that it's never imported.

giladgray · 2018-04-16T23:32:11Z

@reiv take a peek at #2394: i wrote blueprint-icon-components TSLint rule to allow users to enforce component-only usage to enable easy tree shaking. does that seem sufficient? might be able to remove the global flag you've added in favor of enabling that lint rule?

reiv · 2018-04-17T00:44:53Z

@giladgray I'll try to address your code comments later today. As for that global flag, I'm quite certain it's still needed. The default behavior is to pull in all icon components when importing Icon.tsx, in order to continue supporting the use of strings in the icon prop (we need a mapping of name strings to actual icon components). Doing this would disable tree-shaking regardless of whether the user actually uses the string form or goes the icon component route -- hence the need to wrap the thing up in an if block which is then statically removed by dead code elimination at compile time. It's not pretty, but I don't think there's another way without breaking backwards compatibility. If anyone can think of an alternative mechanism that works without a compile flag, please do let me know.

Edit: lint rule looks good by the way. An auto-fixer would be the icing on top of course!

reiv · 2018-04-17T10:07:18Z

Move component-specific props out of common

Preview: documentation | landing | table

reiv · 2018-04-29T08:02:40Z

@giladgray Just letting you know that I won't have the time to work on this PR for the next few weeks due to other commitments. Sorry about that! Please feel free to take over from here (or cherry-pick into your own branch).

giladgray · 2018-05-01T00:35:13Z

@reiv thanks for the update! i'm sorry for not having had the time to properly review this, but i will certainly make sure it gets released soon, likely as part of 3.0

jonavila · 2018-06-21T12:40:07Z

@giladgray Any progress with this PR?

giladgray · 2018-06-21T16:48:25Z

@jonavila nope it's not a priority for us internally (we have bigger fish to tree-shake) and it can be done in a non-breaking way so not critical for 3.0.0. there may be some progress soon 😄

ghost · 2018-06-21T17:08:40Z

What do you mean with we have bigger fish to tree-shake?

giladgray · 2018-06-21T18:56:51Z

@jonavila we as a company use the icons widely and frequently in a parameterized way (hence the IconName string literals), so we can't actually tree shake any of them. as a result this feature doesn't have much direct value to Palantir, so it's low on our priority list.

jonavila · 2018-06-21T18:59:23Z

@giladgray Totally get that. I think your answer was directed to @johnunclesam

giladgray · 2018-06-21T22:23:10Z

ah yep it was, thanks 🌴

ghost · 2018-10-09T18:19:44Z

Breaking change

giladgray · 2019-01-18T00:41:56Z

gonna close this PR because woo boy is it out of date!

frederikhors · 2019-01-18T09:16:40Z

gonna close this PR because woo boy is it out of date!

It is out of date, ok, @giladgray, but is tree-shaking for SVG icons a thing in blueprint today?

giladgray · 2019-01-18T18:44:43Z

@frederikhors nope, sorry. this feature is sadly not a priority for the team.

reiv added 12 commits April 3, 2018 17:12

Pull buildPathsObject into buildPaths

f69463b

Generate individual icon modules

1bb1544

Add IconModule interface

dd95aef

Generate allIcons.ts

11710f3

Disable tslint in generated icon modules

408f7d0

Generate svgIcons.ts with individual icon exports

d7d5a2f

Export SVG icons from module root

679ef83

Refactor Icon to accept SVGIcon as icon prop

7a4d707

Remove iconSvgPaths.ts export

c37e6de

Fix issue with at-loader on Windows

c45e459

Don't return undefined in render

2c07bf0

Bail if icon name is invalid

41421aa

llorca requested a review from giladgray April 3, 2018 23:48

giladgray reviewed Apr 5, 2018

View reviewed changes

Refactor for code style

60069f3

giladgray reviewed Apr 5, 2018

View reviewed changes

Fix whitespace

2b41f08

Revert "Fix issue with at-loader on Windows"

8f21779

This reverts commit c45e459.

reiv added 5 commits April 6, 2018 23:47

Fix async reduce

dc87059

Move Icon from core to icons package

ac4f4cb

Duplicate minimal subset of core/common

ce86ff9

Move iconName.ts and svgIcon.ts to common

70846df

Fix Classes export

7670126

This was referenced Apr 16, 2018

tslint rules to ease migration #2393

Closed

add new lint rules to ease migration #2394

Merged

giladgray reviewed Apr 16, 2018

View reviewed changes

reiv added 7 commits April 17, 2018 11:18

Merge branch 'develop' into rv/icon-tree-shaking

10ce268

Remove duplicate error message

bba99f7

Update copyrights

9708e64

Fix indentation

2249959

Update error message wording

aa1ba46

Reference SVGIcon for icon sizes

bbc3efb

Move component-specific props out of common

c5492e7

mshwery mentioned this pull request Nov 27, 2018

Large bundle size with webpack v4 segmentio/evergreen#458

Closed

giladgray closed this Jan 18, 2019

skovy mentioned this pull request Jul 25, 2019

A way to only bundle icons you actually use #2193

Closed

adidahiya mentioned this pull request Jan 22, 2020

chore: convert custom tslint rules to eslint #3917

Merged

		@@ -0,0 +1,71 @@
		/*
		* Copyright 2015 Palantir Technologies, Inc. All rights reserved.


		let allIcons: { [key: string]: IconPartial \| undefined } \| null = null;

		if (!(global as any).BLUEPRINT_ICONS_TREE_SHAKING) {

[icons] Enable tree-shaking for SVG icons #2356

[icons] Enable tree-shaking for SVG icons #2356

Conversation

reiv commented Apr 3, 2018 • edited Loading

Fixes #2193

Changes proposed in this pull request:

reiv commented Apr 3, 2018

Bail if icon name is invalid

giladgray left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

reiv Apr 5, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

reiv commented Apr 5, 2018 • edited Loading

reiv commented Apr 5, 2018

Refactor for code style

giladgray commented Apr 5, 2018

Choose a reason for hiding this comment

reiv commented Apr 5, 2018

reiv commented Apr 5, 2018

Fix whitespace

reiv commented Apr 6, 2018 • edited Loading

giladgray left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

giladgray commented Apr 16, 2018

reiv commented Apr 17, 2018 • edited Loading

reiv commented Apr 17, 2018

Move component-specific props out of common

reiv commented Apr 29, 2018

giladgray commented May 1, 2018

jonavila commented Jun 21, 2018

giladgray commented Jun 21, 2018

ghost commented Jun 21, 2018

giladgray commented Jun 21, 2018 • edited Loading

jonavila commented Jun 21, 2018

giladgray commented Jun 21, 2018

ghost commented Oct 9, 2018

giladgray commented Jan 18, 2019

frederikhors commented Jan 18, 2019

giladgray commented Jan 18, 2019

reiv commented Apr 3, 2018 •

edited

Loading

reiv Apr 5, 2018 •

edited

Loading

reiv commented Apr 5, 2018 •

edited

Loading

reiv commented Apr 6, 2018 •

edited

Loading

reiv commented Apr 17, 2018 •

edited

Loading

giladgray commented Jun 21, 2018 •

edited

Loading