palantir / blueprint

A React-based UI toolkit for the web
https://blueprintjs.com/
Apache License 2.0
20.74k stars 2.18k forks source link

A way to only bundle icons you actually use #2193

Closed jrmyio closed 1 year ago

jrmyio commented 6 years ago

Is there currently no way to only bundle the icons you are actually using?

When importing the Icon Component: import { Popover } from '@blueprintjs/core';

It adds a whopping ~350kb to your build because the entire iconSVGPaths.js is included. Tree shaking with Webpack 4 doesn't fix this.

Is this intended? Are there any workarounds to only bundle icons actually used?

adidahiya commented 6 years ago

This is intended in 2.0, yes. We considered a few different architectures for SVG icons and decided on the current one for the best upgrade story (sorry we didn't share the RFCs for @blueprintjs/icons externally; we'll try to do so in the future).

I would be open to ideas for changing the architecture if you can get webpack 4 tree shaking to work in to eliminate unused icons from your bundle -- there is still some time for breaking changes because we haven't released 2.0.0 yet.

reiv commented 6 years ago

I've been thinking about this a bit, and I think this is an approach that could work:

The idea would be to split up the icons into separate files; each file would be named after the icon and export just the path data needed to render that specific icon, and the icon name, at the module level.

// asterisk.ts
export name = "asterisk";
export path16 = ["M14.54 11.18l.01-.02L9.8 ..."]
export path20 = ["M18.52 14.171.01-.02L11.89 ..."]

Such a module could be described by an interface, like:

export interface IconDefinition {
    /** Icon description for a11y */
    name: string;
    /** SVG paths */
    path16: string[];
    path20: string[];
}

These icons could live in the icons package. A separate all.ts would simply be a re-export of every one of these individual files.

The Icon component in @blueprintjs/core would need to be expanded to also accept IconDefinition:

export interface IIconProps extends IIntentProps, IProps {
    ...
    icon: IconName | IconDefinition | JSX.Element | false | null | undefined;
    ...

To use the icon, one would import the file directly, e.g. import * as Asterisk from "@blueprintjs/icons/asterisk"; for convenience, one could write a dedicated icons.ts which would just export all icons used in that project.

Finally, there would need to be a way to prevent the entirety of the icons package from being required by default (I believe this happens as soon as Icon is imported, either directly or as part of a component that uses it). One way might be to wrap the import in an if expression that checks for some variable being set, like so:

// in Icon.tsx

import * as _allIcons from "@blueprintjs/icons/all";
let allIcons: typeof _allIcons | undefined = undefined;

if (!BLUEPRINT_INDIVIDUAL_ICONS) {
    allIcons = require("@blueprintjs/icons/all");
}

I believe this is enough for Webpack's tree shaking to kick in. The variable could either be global or added by Webpack's DefinePlugin.

Of course, not pulling in all icons at once means that the IconName form for the icon prop can no longer be used, so a runtime error should be issued if icon is specified as a string.

For maximum convenience, a Babel plugin could automagically transform string-form icon props into the corresponding imports (this is how lodash does it, for example.)

Since all this is purely additive, there wouldn't be any breaking changes.

Would love to hear your thoughts on this.

reiv commented 6 years ago

Perhaps splitting up the icons into individual files is a little overkill (it does seem to add a lot of overhead for Webpack too!) -- just providing the icons as individual exports rather than in continuous arrays would probably do the trick as well. Still trying things out on my end, but I'll PR as soon as I have something workable.

giladgray commented 6 years ago

this does not require API breaks so it will not happen in 3.0.0

parisholley commented 6 years ago

@ConneXNL I worked around this in webpack by swapping the file with a custom version that only includes the icons I need:

new webpack.NormalModuleReplacementPlugin(
        /iconSvgPaths.js/,
        `${root}/iconSvgPaths.js`
)
Ameobea commented 6 years ago

@parisholley I've tried this on my own project, but I wasn't able to get it working. I've tried tweaking the regex to /.*iconSvgPaths.[jt]s/ in an attempt to match correctly, but nothing seems to be working.

Did you make any other changes to your webpack conf to make this work?

parisholley commented 6 years ago

@Ameobea nope, just follow w/e instructions exist for the webpack version you are using

https://webpack.js.org/plugins/normal-module-replacement-plugin/

i'm on the latest everything atm

crysehillmes commented 6 years ago

@Ameobea Use /.*\/generated\/iconSvgPaths.*/.

jasonbarry commented 6 years ago

Here's how I got it working with a next.js project. Also had to install file-loader for the build to succeed. Main bundle is 211KB smaller now.

// /next.config.js
const path = require('path');
const { NormalModuleReplacementPlugin } = require('webpack');
// ...
config.plugins.push(
  new NormalModuleReplacementPlugin(
    /.*\/generated\/iconSvgPaths.*/,
    path.resolve(__dirname, 'test/emptyModule.js'),
  )
)
// /test/emptyModule.js
export default '';
nucab commented 5 years ago

@jasonbarry I got this error. Did you encounter this error?

Attempted import error: 'IconSvgPaths16' is not exported from './generated/iconSvgPaths'.
ernestofreyreg commented 5 years ago

Solved for nextjs by:

// /next.config.js
const path = require('path')
const { NormalModuleReplacementPlugin } = require('webpack')

module.exports = {
  webpack: (config, { buildId, dev, isServer, defaultLoaders }) => {
    config.plugins.push(
      new NormalModuleReplacementPlugin(
        /.*\/generated\/iconSvgPaths.*/,
        path.resolve(__dirname, 'lib/icons.js'),
      )
    )

    return config
  }
}
// /lib/icons.js

export const IconSvgPaths16 = {
    "cross": ["M9.41 8l3.29-3.29c.19-.18.3-.43.3-.71a1.003 1.003 0 0 0-1.71-.71L8 6.59l-3.29-3.3a1.003 1.003 0 0 0-1.42 1.42L6.59 8 3.3 11.29c-.19.18-.3.43-.3.71a1.003 1.003 0 0 0 1.71.71L8 9.41l3.29 3.29c.18.19.43.3.71.3a1.003 1.003 0 0 0 .71-1.71L9.41 8z"],
    "lock": ["M13.96 7H12V3.95C12 1.77 10.21 0 8 0S4 1.77 4 3.95V7H1.96c-.55 0-.96.35-.96.9v6.91c0 .54.41 1.19.96 1.19h12c.55 0 1.04-.65 1.04-1.19V7.9c0-.55-.49-.9-1.04-.9zM6 7V3.95c0-1.09.9-1.97 2-1.97s2 .88 2 1.97V7H6z"],
    "user": ["M7.99-.01A7.998 7.998 0 0 0 .03 8.77c.01.09.03.18.04.28.02.15.04.31.07.47.02.11.05.22.08.34.03.13.06.26.1.38.04.12.08.25.12.37.04.11.08.21.12.32a6.583 6.583 0 0 0 .3.65c.07.14.14.27.22.4.04.07.08.13.12.2l.27.42.1.13a7.973 7.973 0 0 0 3.83 2.82c.03.01.05.02.07.03.37.12.75.22 1.14.29l.2.03c.39.06.79.1 1.2.1s.81-.04 1.2-.1l.2-.03c.39-.07.77-.16 1.14-.29.03-.01.05-.02.07-.03a8.037 8.037 0 0 0 3.83-2.82c.03-.04.06-.08.09-.13.1-.14.19-.28.28-.42.04-.07.08-.13.12-.2.08-.13.15-.27.22-.41.04-.08.08-.17.12-.26.06-.13.11-.26.17-.39.04-.1.08-.21.12-.32.04-.12.08-.24.12-.37.04-.13.07-.25.1-.38.03-.11.06-.22.08-.34.03-.16.05-.31.07-.47.01-.09.03-.18.04-.28.02-.26.04-.51.04-.78-.03-4.41-3.61-7.99-8.03-7.99zm0 14.4c-1.98 0-3.75-.9-4.92-2.31.67-.36 1.49-.66 2.14-.95 1.16-.52 1.04-.84 1.08-1.27.01-.06.01-.11.01-.17-.41-.36-.74-.86-.96-1.44v-.01c0-.01-.01-.02-.01-.02-.05-.13-.09-.26-.12-.39-.28-.05-.44-.35-.5-.63-.06-.11-.18-.38-.15-.69.04-.41.2-.59.38-.67v-.06c0-.51.05-1.24.14-1.72.02-.13.05-.26.09-.39.17-.59.53-1.12 1.01-1.49.49-.38 1.19-.59 1.82-.59.62 0 1.32.2 1.82.59.48.37.84.9 1.01 1.49.04.13.07.26.09.4.09.48.14 1.21.14 1.72v.07c.18.08.33.26.37.66.03.31-.1.58-.16.69-.06.27-.21.57-.48.62-.03.13-.07.26-.12.38 0 .01-.01.04-.01.04-.21.57-.54 1.06-.94 1.42 0 .06.01.13.01.19.04.43-.12.75 1.05 1.27.65.29 1.47.6 2.14.95a6.415 6.415 0 0 1-4.93 2.31z"],
    "warning-sign": ["M15.84 13.5l.01-.01-7-12-.01.01c-.17-.3-.48-.5-.85-.5s-.67.2-.85.5l-.01-.01-7 12 .01.01c-.09.15-.15.31-.15.5 0 .55.45 1 1 1h14c.55 0 1-.45 1-1 0-.19-.06-.35-.15-.5zm-6.85-.51h-2v-2h2v2zm0-3h-2v-5h2v5z"],
}

export const IconSvgPaths20 = {
    "cross": ["M11.41 10l4.29-4.29c.19-.18.3-.43.3-.71a1.003 1.003 0 0 0-1.71-.71L10 8.59l-4.29-4.3a1.003 1.003 0 0 0-1.42 1.42L8.59 10 4.3 14.29c-.19.18-.3.43-.3.71a1.003 1.003 0 0 0 1.71.71l4.29-4.3 4.29 4.29c.18.19.43.3.71.3a1.003 1.003 0 0 0 .71-1.71L11.41 10z"],
    "lock": ["M15.93 9H14V4.99c0-2.21-1.79-4-4-4s-4 1.79-4 4V9H3.93c-.55 0-.93.44-.93.99v8c0 .55.38 1.01.93 1.01h12c.55 0 1.07-.46 1.07-1.01v-8c0-.55-.52-.99-1.07-.99zM8 9V4.99c0-1.1.9-2 2-2s2 .9 2 2V9H8z"],
    "user": ["M10 0C4.48 0 0 4.48 0 10c0 .33.02.65.05.97.01.12.03.23.05.35.03.2.05.4.09.59.03.14.06.28.1.42l.12.48c.05.16.1.31.15.46.05.13.09.27.15.4.06.16.13.32.21.48.05.11.1.22.16.33.09.17.17.34.27.5.05.09.1.17.15.25.11.18.22.35.34.52.04.06.08.11.12.17 1.19 1.62 2.85 2.86 4.78 3.53l.09.03c.46.15.93.27 1.42.36.08.01.17.03.25.04.49.07.99.12 1.5.12s1.01-.05 1.5-.12c.08-.01.17-.02.25-.04.49-.09.96-.21 1.42-.36l.09-.03c1.93-.67 3.59-1.91 4.78-3.53.04-.05.08-.1.12-.16.12-.17.23-.35.34-.53.05-.08.1-.16.15-.25.1-.17.19-.34.27-.51.05-.11.1-.21.15-.32.07-.16.14-.32.21-.49.05-.13.1-.26.14-.39.05-.15.11-.31.15-.46.05-.16.08-.32.12-.48.03-.14.07-.28.1-.42.04-.19.06-.39.09-.59.02-.12.04-.23.05-.35.05-.32.07-.64.07-.97 0-5.52-4.48-10-10-10zm0 18a7.94 7.94 0 0 1-6.15-2.89c.84-.44 1.86-.82 2.67-1.19 1.45-.65 1.3-1.05 1.35-1.59.01-.07.01-.14.01-.21-.51-.45-.93-1.08-1.2-1.8l-.01-.01c0-.01-.01-.02-.01-.03a4.42 4.42 0 0 1-.15-.48c-.33-.07-.53-.44-.61-.79-.08-.14-.23-.48-.2-.87.05-.51.26-.74.49-.83v-.08c0-.63.06-1.55.17-2.15.02-.17.06-.33.11-.5.21-.73.66-1.4 1.26-1.86.62-.47 1.5-.72 2.28-.72.78 0 1.65.25 2.27.73.6.46 1.05 1.12 1.26 1.86.05.16.08.33.11.5.11.6.17 1.51.17 2.15v.09c.22.1.42.33.46.82.04.39-.12.73-.2.87-.07.34-.27.71-.6.78-.04.16-.09.33-.15.48 0 .01-.02.05-.02.05-.26.71-.67 1.33-1.17 1.78 0 .08.01.16.01.23.05.54-.15.94 1.31 1.59.81.36 1.84.74 2.68 1.19A7.958 7.958 0 0 1 10 18z"],
    "warning-sign": ["M19.86 17.52l.01-.01-9-16-.01.01C10.69 1.21 10.37 1 10 1s-.69.21-.86.52l-.01-.01-9 16 .01.01c-.08.14-.14.3-.14.48 0 .55.45 1 1 1h18c.55 0 1-.45 1-1 0-.18-.06-.34-.14-.48zM11 17H9v-2h2v2zm0-3H9V6h2v8z"],
}

This way you can pick and choose the icons you actually use from the generated file.

01dr commented 5 years ago

Hey! Is there any temporary solution to completely exclude icons from the bundle in create-react-app without ejecting?

Ameobea commented 5 years ago

@amazing-space-invader There are some hacky things you can do:

You can fork the blueprint repo, deleting all of the unused icons from the source code, and pull in the fork as a dependency instead.

Or you can just add a step to your build process that overwrites the icons file in your node_modules directory with a custom file that has all of the unused icons removed.

dphilipson commented 5 years ago

@amazing-space-invader In addition to what @Ameobea said, there's a couple of libraries that allow you to mess with the create-react-app config without ejecting. I just got this icon bundling thing to work in my project using Craco, and it should work about the same in similar CRA config libraries like react-app-rewired.

To get it working with Craco, I followed the install instructions in the Craco readme, then added a craco.config.js with contents:

const path = require("path");
const { NormalModuleReplacementPlugin } = require("webpack");

module.exports = {
  webpack: {
    plugins: [
      new NormalModuleReplacementPlugin(
        /.*\/generated\/iconSvgPaths.js/,
        path.resolve(__dirname, "emptyIconSvgPaths.js"),
      ),
    ],
  },
};

and also added a file emptyIconSvgPaths.js with contents

export const IconSvgPaths16 = {};
export const IconSvgPaths20 = {};

You can see a working example repo here.

Disclaimer: messing with Create React App's config in this way is sketchy and may eventually cause you upgrade pain. Do so at your own risk.

fredfortier commented 5 years ago

@dphilipson I don't feel educated enough about Craco to apply this example directly to the Blueprint. Do you (or anyone else) have an example of selectively importing Blueprint icons?

dphilipson commented 5 years ago

@fredfortier I'll try to help if I can. First of all, Craco or similar libraries are only necessary if you are using the Create React App starter project. The steps for selectively importing icons depend on your project setup.

If you are using Create React App (and haven't ejected):

  1. Install Craco with npm install @craco/craco or yarn add @craco/craco.
  2. In your package.json, replace "react-scripts start", "react-scripts build", and "react-scripts test" with "craco start", "craco build" respectively.
  3. In your project root, create a file named craco.config.js with contents described in my previous post.
  4. Also in your project root, create a file named emptyIconSvgPaths.js with contents described in my previous post.

If you want an example of all these steps in practice, see this commit.

This will bundle the project with no icons. If you want to selectively add certain icons, then add them to emptyIconSvgPaths.js as described in @ernestofrereg's post.

Now if you're not using Create React App, then see if your project has a Webpack configuration file, likely in the project root and named webpack.config.js. Then edit this configuration, adding

      new NormalModuleReplacementPlugin(
        /.*\/generated\/iconSvgPaths.*/,
        path.resolve(__dirname, 'lib/icons.js'),
      )

to the list of plugins (also adding the necessary imports to the top of the file).

Or if you're using Next.js, then try following the instructions in @ernestofreyreg's post in their entirety.

ernestofreyreg commented 5 years ago

You can also create your own icons. You can give it any name and just copy the SVG paths (adjusted for 20x20 and 16x16 viewbox)

skovy commented 5 years ago

I believe the approach @reiv mentioned above could be effective. The Font Awesome packages do something very similar (they also have docs specifically for tree shaking: https://fontawesome.com/how-to-use/with-the-api/other/tree-shaking)

They have two options for using. One, import the icon directly (import { faCoffee } from '@fortawesome/free-solid-svg-icons') which allows tree shaking because webpack can determine which exports are used and rely on minification using the Terser plugin to perform the dead code elimination. Or, if not using a tool that supports tree-shaking you can do a "deep import": import { faCoffee } from '@fortawesome/free-solid-svg-icons/faCoffee'.

The other option they provide is to add all the icons (equivalent to what's happening today):

import { library } from '@fortawesome/fontawesome-svg-core'
import { fas } from '@fortawesome/free-solid-svg-icons'
import { far } from '@fortawesome/free-regular-svg-icons'
import { fab } from '@fortawesome/free-brands-svg-icons'

// Add all icons to the library so you can use it in your page
library.add(fas, far, fab)

(Relevant docs: https://fontawesome.com/how-to-use/with-the-api/setup/importing-icons#icon-names)

Here is an example of what the esm source looks like: https://unpkg.com/@fortawesome/free-solid-svg-icons@5.9.0/index.es.js

Each individual icon is exported to enable the above tree shaking, but the _iconCache as fas value is also exported which includes all the icons so for those that are no interested in tree shaking it's still easy to use the library.add(fas) API.

@giladgray @adidahiya would you be open to a proof-of-concept PR with these changes?

Edit: oh wow, I missed the original attempt here: https://github.com/palantir/blueprint/pull/2356 😅

ashi009 commented 5 years ago

Isn't the most straightforward solution is to export those paths directly without using the names? By dropping that layer of indirection, this can be done with just standard ES module semantic. Bundlers will drop unused icons as unused variables, without the help of special loaders or magic function calls.

@blueprint/icons/index.ts:

function define(name: string, path16: string[], path20: string[]): IconDefinition { return { name, path16, path20 }; }

export const SOME_ICON = /*#__PURE__*/define("some_icon", [...], [...]);
export const SOME_UNUSED_ICON = /*#__PURE__*/define("some_unused_icon", [...], [...]);

some.tsx:

import * as icons from '@blueprint/icons';

<Icon icon={icons.SOME_ICON} />
temaivanoff commented 4 years ago

HI, i use only one component contextmenu and 3 menu items with icons Снимок экрана 2020-02-05 в 13 25 18

I will also use menu items with icons in Material-UI, they use separate files for icons Снимок экрана 2020-02-05 в 13 40 04

adidahiya commented 3 years ago

I am picking up this work again for Blueprint 4.0 (targeted for early 2021). In general, I am aligned with the approach used in fontawesome as described above by @skovy in https://github.com/palantir/blueprint/issues/2193#issuecomment-514840002.

I'd like to preserve the convenient developer experience of not having to import every icon symbol from "@blueprintjs/icons" whenever an icon is specified in as component API (for example <Button icon="refresh" />).

But I'd like also provide the option to leverage webpack/rollup tree shaking. A package global singleton approach seems like it could work:

import { Icons } from "@blueprintjs/icons";

// for access to all icons across BP packages
Icons.loadAll();

// for limited access to specific icons
Icons.load([
  "refresh",
  ...
]);

but that will be difficult to enforce safely in the type system. I guess we might be ok with saying that the string enum IconName API may fail at runtime... we could log some kind of dev warning for it:

if (!Icons.isLoaded(iconName)) {
  if (!isProduction()) {
    console.error(`You attempted to use an icon without loading it. Ensure that ${iconName}` is loaded for this instance of the "@blueprintjs/icons" package.`);
  }
}
marcelltoth commented 3 years ago

... or use the even cleaner approach that FA also supports:

import { someIcon } from '@blueprintjs/icons';

// in a component
<Button icon={someIcon} />

Then tree shaking is trivial, and implementation isn't much more complex either. You just have to export icon SVG data from an ES6 module, but it seems you already have that under the hood

While I understand your point and I'm sure some developers feel the same

I'd like to preserve the convenient developer experience of not having to import every icon symbol from "@blueprintjs/icons" whenever an icon is specified in as component API (for example <Button icon="refresh" />).

I much prefer cleanly importing things and having type-checking, rather than causing global side-effects with a global singleton and receiving potential runtime errors when I forget to add one. Especially when developing libraries built on top of blueprint, one really wants to remain side-effect-free and the library approach does not allow that unfortunately.

adidahiya commented 3 years ago
import { someIcon } from '@blueprintjs/icons';

// in a component
<Button icon={someIcon} />

@marcelltoth yes, I'd still want to support that kind of API where tree shaking is trivial. someIcon would be an <svg> of type JSX.Element and you'd be guaranteed to load it via your module bundler.

We have different use cases to support. Blueprint's main use case in Palantir applications occurs in an environment where we load all icons onto the page by default; we accept the bundle size cost of loading all the icons in our applications because we expect to use many of them. In this environment we'd like to enjoy the developer convenience of the icon: IconName string enum API.

For other users of Blueprint, the performance tradeoff is different and I understand why you might not want to load all icons on the page. I think we can achieve an API which works for both kinds of Blueprint icons consumers rather than compromising too much.

adidahiya commented 3 years ago

I have a preliminary implementation of async icon loading here, follow this PR for updates: https://github.com/palantir/blueprint/pull/4513

adidahiya commented 3 years ago

v4.0.0-alpha.0 has just been released which includes the icons infrastructure changes from #4513. Please try it out and let me know what you think; preferably open new issues with specific concerns about the new APIs.

4.x changelog, docs

silverwind commented 2 years ago

Did anyone figure out how to exclude all icons via vite and/or rollup?

justinbhopper commented 2 years ago

But I'd like also provide the option to leverage webpack/rollup tree shaking. A package global singleton approach seems like it could work:

import { Icons } from "@blueprintjs/icons";

// for access to all icons across BP packages
Icons.loadAll();

// for limited access to specific icons
Icons.load([
  "refresh",
  ...
]);

but that will be difficult to enforce safely in the type system.

One option to consider is requiring that consumers of BlueprintJS include a .d.ts reference (like Create React App or Vite.js do) to import the type definitions. This way, the definition of any control that has icon props can be extended to include the IconName type definition or not, depending on whether they plan to call loadAll() or their own custom set.

For example

@blueprintjs/icons index.d.ts (loaded by default)

export type IconReference = MaybeElement; // Only supports direct SVGs or elements by default

@blueprintjs/icons all.d.ts (opt-in only, see below)

declare type AllIconNames = "chevron-left" | "chevron-right" | etc;
declare type IconReference = MaybeElement | AllIconNames;

@blueprintjs/core InputGroup.ts

export interface InputGroupProps {
  icon?: IconReference; 
}

In a consumer app that wants all icons bundled (see all.d.ts above): my-app/blueprintjs-icons.d.ts

/// <reference types="@blueprintjs/icons/all" />

In a consumer app that wants only specific icon: my-app/blueprintjs-icons.d.ts

declare module "@blueprintjs/icons" {
  type IconReference = MaybeElement | "comment" | "person";
}

This has some key benefits:

  1. The breaking change is minimal. Users who want the old behavior only need to define a new .d.ts file that has a single line.
  2. Users who want fine control can have it.
  3. There's even an option for users who only want the import-style and don't ever want to use the IconName union type feature.

You could go further with this by creating a new package called @blueprintjs/icons-bundled (or @types/blueprintjs-all-icons?) which automatically defines the extension to IconReference automatically.

ro-savage commented 2 years ago

For anyone using Blueprint 4.x - this is the update code to ignore the icons.

Update your webpack config to

    new NormalModuleReplacementPlugin(
      /.*\/@blueprintjs\/icons\/lib\/esm\/iconSvgPaths.*/,
      resolve(__dirname, "../../app/javascript/blueprint-icons.js"),
    ),

Then inside your blueprint-icons.js add

Either this if you are going to keep using the old warning-sign naming

export function iconNameToPathsRecordKey(name) {
  return name
}

or this if you want to rename all your already imported icons to PascalCase

function toPascalCase(string) {
  return `${string}`
    .toLowerCase()
    .replace(new RegExp(/[-_]+/, 'g'), ' ')
    .replace(new RegExp(/[^\w\s]/, 'g'), '')
    .replace(
      new RegExp(/\s+(.)(\w*)/, 'g'),
      ($1, $2, $3) => `${$2.toUpperCase() + $3}`
    )
    .replace(new RegExp(/\w/), s => s.toUpperCase());
}

export function iconNameToPathsRecordKey(name) {
  return toPascalCase(name);
}
switz commented 2 years ago

This ignores the icons, but it doesn't dynamically import/code split them, unless I'm misunderstanding how you're doing this.

adidahiya commented 1 year ago

@blueprintjs/core & @blueprintjs/icons v5.0.0-alpha.1 have support for tree shaking and dynamically importing icons. Please try it out and let me know how it goes.

@justinbhopper's idea in https://github.com/palantir/blueprint/issues/2193#issuecomment-1188473805 looks interesting; I will try that out but no guarantees that I'll be able to include this additional feature of statically declaring a subset of available icons in type definitions in the v5.0 release. It might get punted to v6.0.

justinbhopper commented 1 year ago

@adidahiya ~The new version generates components for each icon, but each of these components contains an entire SVG structure. Wouldn't it be easier if they only generated a React Fragment that contained the <path> element?~

~That way, most of the SVG structure (and all its custom behavior like attributes, classnames, title element, etc) can remain in the control of the <Icon> component.~

~In fact, I'm not entirely sure I understand why a component had to be pre-generated in the first place. Why not just dynamically import the path data?~

justinbhopper commented 1 year ago

@adidahiya I looked up your original PR #4513 and realize one of your goals was to have the icons be exportable as their own individual component, e.g. import { Download } from "@blueprintjs/icons", which explains why you chose to have a component be pre-generated. So please ignore the comment above.

I think it's likely there are consumers of the old IconSvgPaths16 and IconSvgPaths20 exports (our team is one of them). I can give details on why we used the paths directly, but that's probably not relevant here.

Could we make it possible to still retrieve the path data using the new Icons icons loader? e.g. Icons.getPath(icon: IconName).

adidahiya commented 1 year ago

Statically loading all icon paths

You can currently access icon paths like this (in both v4.x and v5.0) where all icons paths will be imported into your bundle:

import { IconSvgPaths16, iconNameToPathsRecordKey } from "@blueprintjs/icons";

IconSvgPaths16[iconNameToPathsRecordKey("caret-down")];

Admittedly this is a little verbose, and it would be nicer to have an API like this:

import { getIconPath } from "@blueprintjs/icons";

getIconPath("caret-down", 16);

I'll add that new function in v5.0.

Dynamically loading icon paths

Could we make it possible to still retrieve the path data using the new Icons icons loader? e.g. Icons.getPath(icon: IconName).

Yes, it would be possible to add a new API to IconLoader which loads icon path modules dynamically. I will consider adding Icons.getPath(icon: IconName) in v5.0.

Note that this will require adding another customization option to IconLoader so that users can load the path modules with non-default webpack import options and in other bundlers which are not webpack (example: https://github.com/palantir/blueprint/issues/6152).

@blueprintjs/icons package size

I noticed that the new icons build system results in a pretty large NPM tarball with a ton of files in v5.0:

npm notice === Tarball Details ===
npm notice name:          @blueprintjs/icons
npm notice version:       5.0.0-alpha.4
npm notice filename:      blueprintjs-icons-5.0.0-alpha.4.tgz
npm notice package size:  3.6 MB
npm notice unpacked size: 22.6 MB
npm notice shasum:        522c11c99d6efcb7bb31f11c14bbbcf94950d6b8
npm notice integrity:     sha512-hxBOhQbaXdRya[...]U8tYZvZUIrb6w==
npm notice total files:   17640

This is the result of attempting to support many different ways to export tree-shakable icon components & paths. I don't have a concrete plan right now to mitigate this tarball size or number of files, but just wanted to put it out there that I'm aware of this size inflation.

adidahiya commented 1 year ago

I decided to change the icon loading API in #6212 based on @justinbhopper's suggestion. Icons.load() now loads paths instead of components, and shared logic for rendering the <svg> element container now lives in <SVGIconContainer>.

Bundle size is slightly reduced after de-duplicating some of the generated code:

npm notice === Tarball Details === 
npm notice name:          @blueprintjs/icons                      
npm notice version:       5.0.0-beta.0                            
npm notice filename:      blueprintjs-icons-5.0.0-beta.0.tgz      
npm notice package size:  3.5 MB                                  
npm notice unpacked size: 18.4 MB                                 
npm notice shasum:        2c73c2de5826b1a4e446c7d2eec486d0ebc62fa2
npm notice integrity:     sha512-IDgwYepbxLgCL[...]m+GBvXeudfT/Q==
npm notice total files:   17650 

I'm going to close this out as complete, and I'm signaling that v5.0 is basically done with the 5.0.0-beta.0 version I just released. We can discuss further improvements to icons in new issue threads.