Skip to content
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

[v4] load icons asynchronously #4513

Merged
merged 17 commits into from
Feb 9, 2021
Merged

[v4] load icons asynchronously #4513

merged 17 commits into from
Feb 9, 2021

Conversation

adidahiya
Copy link
Contributor

@adidahiya adidahiya commented Feb 1, 2021

Fixes #2193

Checklist

  • Includes tests
  • Update documentation

Changes proposed in this pull request:

  • All packages now build to a "es2020" module target. This is nearly identical to the previous "es2015" target, but now has support for dynamic import() expressions (MDN docs).
    • ⚠️ You will have to ensure that your build system is able to transpile / handle this new ES syntax. If you are using modern browsers, modern Node.js, modern Webpack, and/or @babel/plugin-syntax-dynamic-import, this should require no action on your part. In other words, most modern tooling in the JS ecosystem does support this new feature.
  • Expose a singleton class API { Icons } from "@blueprintjs/icons" to load specific icons from the icon set
  • Expose individual icon SVG component modules, e.g. import { Download } from "@blueprintjs/icons"
  • <Icon> component's static SVG path string loader has been replaced by a loader that uses a dynamic import() expression
  • Because of the dynamic import change, I have changed all BP component source code to use static icon module imports instead of the string literal API. This means they will work out of the box without any user configuration.

Reviewers should focus on:

  • Developer experience of icon loader
  • Performance of dynamic import

Screenshot

@adidahiya adidahiya changed the title [v4] breaking: refactor icons build pipeline to use fantasticon (#4505) [v4] load icons asynchronously Feb 1, 2021
@blueprint-bot
Copy link

Merge branch 'v4' into ad/icons-loading

Previews: documentation | landing | table

@blueprint-bot
Copy link

fix icon test suite

Previews: documentation | landing | table

@blueprint-bot
Copy link

fix iso test compilation

Previews: documentation | landing | table

@blueprint-bot
Copy link

fix tests, dist

Previews: documentation | landing | table

@adidahiya adidahiya merged commit 1c1eef5 into v4 Feb 9, 2021
@adidahiya adidahiya deleted the ad/icons-loading branch February 9, 2021 19:19
@adidahiya adidahiya added this to the 4.0.0-beta.0 milestone Mar 4, 2021
@switz
Copy link
Contributor

switz commented Apr 15, 2021

I'm seeing every icon imported into my nextjs app on 4.0.0. Is there a specific API I should be using to treeshake them dynamically? https://nextjs.org/docs/advanced-features/dynamic-import

Before I was overriding the generated/svgPaths file. I generally specify my icons via: <Icon icon="caret-down" /> but I can switch to explicit imports if that helps.

image

@adidahiya
Copy link
Contributor Author

@switz can you open a new issue and we can try to debug from there? you shouldn't need to configure anything if you're using webpack (which Next.js does use under the hood); this bit of code should be doing the dynamic import for you:

/**
* The default icon contents loader implementation, optimized for webpack.
*
* @see https://webpack.js.org/api/module-methods/#magic-comments for dynamic import() reference
*/
const defaultIconContentsLoader: IconComponentLoader = async name => {
return (
await import(
/* webpackInclude: /\.js$/ */
/* webpackMode: "lazy-once" */
`./generated/components/${name}`
)
).default;
};

@alex-kinokon
Copy link

@adidahiya Is this change reversed? import { Download } from "@blueprintjs/icons" no longer works for 4.0.0-beta.2.

@adidahiya
Copy link
Contributor Author

@proteriax yes, see my comment #4645 (comment)

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants