-
Notifications
You must be signed in to change notification settings - Fork 2.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[icons] Enable tree-shaking for SVG icons #2356
Changes from 12 commits
f69463b
1bb1544
dd95aef
11710f3
408f7d0
d7d5a2f
679ef83
7a4d707
c37e6de
c45e459
2c07bf0
41421aa
60069f3
2b41f08
8f21779
dc87059
ac4f4cb
ce86ff9
70846df
7670126
1dd1f8d
8471203
5e50f47
ef54225
53b5ead
bffc817
d4f2b89
3359402
6e5dbb0
770c186
5876f6f
6827ec1
10ce268
bba99f7
9708e64
2249959
aa1ba46
bbc3efb
c5492e7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 = | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. is this used in |
||
ns + | ||
` Specifying icon by string name is not supported because the BLUEPRINT_ICONS_TREE_SHAKING flag has been set.` + | ||
` Icons must be imported as individual modules from the @blueprintjs/icons package.`; | ||
|
||
export const NUMERIC_INPUT_MIN_MAX = | ||
ns + ` <NumericInput> requires min to be strictly less than max if both are defined.`; | ||
export const NUMERIC_INPUT_MINOR_STEP_SIZE_BOUND = | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,11 @@ | ||
/* | ||
* Copyright 2018 Palantir Technologies, Inc. All rights reserved. | ||
*/ | ||
|
||
/** | ||
* Compact representation of an SVG icon. | ||
* - `[0]`: name | ||
* - `[1]`: 16px SVG path | ||
* - `[2]`: 20px SVG path | ||
*/ | ||
export type SVGIcon = [string, string[], string[]]; |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -8,6 +8,7 @@ | |
const fs = require("fs"); | ||
const path = require("path"); | ||
const SVGO = require("svgo"); | ||
const lodash = require("lodash"); | ||
const { COPYRIGHT_HEADER } = require("./constants"); | ||
|
||
const svgo = new SVGO({ plugins: [{ convertShapeToPath: { convertArcs: true } }] }); | ||
|
@@ -51,17 +52,21 @@ writeLinesToFile("iconNames.ts", ...exportIconConsts(icon => icon.iconName)); | |
|
||
(async () => { | ||
// SVG path strings. IIFE to unwrap async. | ||
const iconSvgPaths16 = await buildPaths(16); | ||
const iconSvgPaths20 = await buildPaths(20); | ||
|
||
writeLinesToFile( | ||
"iconSvgPaths.ts", | ||
'import { IconName } from "../iconName";', | ||
"", | ||
"export const IconSvgPaths16: Record<IconName, string[]> = {", | ||
...(await buildPathsObject("IconSvgPaths", 16)), | ||
"};", | ||
"svgIcons.ts", | ||
"// tslint:disable:prettier", | ||
'import { SVGIcon } from "../svgIcon"', | ||
"", | ||
"export const IconSvgPaths20: Record<IconName, string[]> = {", | ||
...(await buildPathsObject("IconSvgPaths", 20)), | ||
"};", | ||
...ICONS_METADATA.map(({iconName}) => { | ||
const alias = lodash.camelCase(iconName); | ||
return `export const ${alias}Icon: SVGIcon = [` | ||
+ `"${iconName}"` + "," | ||
+ iconSvgPaths16[iconName] + "," | ||
+ iconSvgPaths20[iconName] + "]" | ||
}), | ||
); | ||
})(); | ||
|
||
|
@@ -101,22 +106,34 @@ function exportIconConsts(valueGetter) { | |
return ICONS_METADATA.map(icon => `export const ${toEnumName(icon)} = "${valueGetter(icon)}";`); | ||
} | ||
|
||
/** | ||
/* | ||
* Loads SVG file for each icon, extracts path strings `d="path-string"`, | ||
* and constructs map of icon name to array of path strings. | ||
* @param {string} objectName | ||
* and returns a map of icon names to stringified arrays of path strings. | ||
* @param {16 | 20} size | ||
*/ | ||
async function buildPathsObject(objectName, size) { | ||
return Promise.all( | ||
async function buildPaths(size) { | ||
const paths = {}; | ||
await Promise.all( | ||
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")}]`; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this is now a
|
||
}), | ||
); | ||
return paths; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. something weird going on with whitespace here? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. indentation? |
||
} | ||
|
||
/** | ||
* Returns stringified contents of a map of icon names to arrays of path | ||
* strings. | ||
* @param {Object} paths - paths as returned by buildPaths | ||
*/ | ||
function buildPathsObject(paths) { | ||
return Object.keys(paths).map(iconName => { | ||
return ` "${iconName}": ${paths[iconName]},` | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. inline! |
||
}); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is this necessary now? we've never needed it in the past.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 findrequire()
. There's some discussion of it here: PatrickJS/PatrickJS-starter#1371It shouldn't affect anything else, but happy to split this off into a separate PR to reduce noise in the diff.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmmm
core
declares therequire()
type. we should probably just do the same in icons rather than changing the tsconfig.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 ofcan't find "it/describe/etc"
errors. I'll definitely submit this as a separate PR then.