-
Notifications
You must be signed in to change notification settings - Fork 13
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
Allow Gridicons library to be imported by component #283
Conversation
To make this possible: import GridiconExternal from 'gridicons/external'; we need to publish the individual icons in the root of our npm package. This also changes how do we tell npm which files to include from whitelisting (using the files prop in package.json) to blacklisting (using the .npmignore file). The reason for that is that I've tried to whitelist every individual icon by setting the files prop to "*.js" and then blacklist only the Gruntfile.js within .npmignore. It seems that in case of conflict the files prop takes precedence over .npmignore, so the Gruntfile.js was still included.
Note that this is necessary to prevent the build/ folder to be included in the npm package. For some reason, although the build/ folder was blacklisted in the .npmignore, it was not excluded from the package. https://docs.npmjs.com/misc/developers#keeping-files-out-of-your-package The docs don't mention the build/ folder as one of the always included directories, though.
By using a function instead of React.PureComponent the transpiled icon is 1,5K smaller. Most individual components are between 4K and 5K, so this change means a ~30% size reduction.
Really interesting, thank you for doing this. Question: can you still import all gridicons at once, so we have backwards compatibility for existing code? |
Yes! |
Just a note for reviewers: this PR probably merits a new release. I can do that after review. |
.npmignore
Outdated
svg-min/ | ||
svg-min-react/ | ||
svg-sprite/ | ||
Gruntfile.js |
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.
Is a blacklist the right approach here? Seems like we only want to publish only comparatively few files in the npm, which to me indicates a whitelist (i.e. the files
attribute in package.json
) is better suited. Curious why you decided against continuing to use that?
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.
I've tried whitelisting every individual icon by setting the files
prop to *.js
and then blacklist only the Gruntfile.js
using the .npmignore
. It seems that in case of conflict the files prop takes precedence over .npmignore, so the Gruntfile.js was still included in the npm package. So I changed it to blacklisting to prevent the Gruntfile.js from being published.
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.
(Related thread: #283 (comment))
IF YOU ARE EDITING gridicon/index.jsx | ||
THEN YOU ARE EDITING A FILE THAT GETS OUTPUT FROM THE GRIDICONS REPO! | ||
DO NOT EDIT THAT FILE! EDIT index-header.jsx and index-footer.jsx instead | ||
OR if you're looking to change now SVGs get output, you'll need to edit strings in the Gruntfile :) |
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.
The filenames in this comment need updating 🙂
package.json
Outdated
@@ -4,7 +4,7 @@ | |||
"main": "build/index.js", | |||
"scripts": { | |||
"build": "grunt --verbose", | |||
"prepublish": "npm run build" | |||
"prepublish": "npm run build && cp react-icons/*.js ." |
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.
Hmm, are we dropping all individual icon components into the package's root dir prior to publishing? That seems a bit unfortunate, especially if we don't have a cleanup job -- they're probably going to linger afterwards, requiring the dev to remove them manually...
Can we continue to keep things contained in dedicated build directories?
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.
Yes, they are moved to the root of the package only before npm publish
(so it doesn't pollute it during development). We need that to do:
import GridiconExternal from 'gridicons/external';
If they are under build, we'll need to include the build
dir in the npm package and the individual icons would be imported like:
import GridiconExternal from 'gridicons/build/external';
AFAIK, it is common practice for libraries that want to export individual components -such as Lodash- to move them to the root for publishing.
One thing we could do, though, is to run a clean task in the postpublish
npm hook.
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.
A postpublish
cleanup hook is somewhat better, but still not perfect 😁-- as e.g. evidenced by this thread.
Too bad package.json
's main
field can't be a directory. Related: nodejs/node#14970
Another idea: Have build/index.js
do
export { default as GridiconExternal } from './external';
export { default as GridiconFoo } from './foo';
export { default as GridiconGarb } from './garb';
If we publish not only the transpiled but also the untranspiled code to npm, consumers can then
import { GridiconExternal, GridiconGarb } from 'gridicons';
For projects that use webpack >= v3, tree shaking will then ensure they're only importing the relevant code (omitting the Gridicons that aren't import
ed).
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.
How can we test this? I tried |
Oh weird, on second try I actually got |
'gridicons-user-circle', | ||
'gridicons-reader-follow', | ||
'gridicons-reader-following' | ||
]; |
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.
I think we can shrink individual *.jsx
file sizes even further if we don't include all the string arrays found here but rather move the logic found here to grunt-tasks/svg-to-react.js
, which will allow us to only include the readily computed class names in the output files.
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.
What's even worse is that these lists are duplicated from ./index.jsx
. Redundancy === maintenance nightmare for everybody who's going to add a new icon. So we should really centralize this information and build individual files statically.
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.
Good suggestion!
I'm glad that you mention the "adding new icon" task as the driving use case for this. At the time it wasn't evident to me how to centralize all that logic without introducing a lot of complexity for that particular use case - and that's the reason I stopped to hear my colleagues' opinions first. I think I may have an idea to solve that.
Maybe we can also re-implement the generation of |
Gruntfile.js
Outdated
@@ -143,7 +147,7 @@ module.exports = function( grunt ) { | |||
cwd: 'svg-min-react/', | |||
src: [ '**/*.svg' ], | |||
filter: 'isFile', | |||
dest: 'build/' | |||
dest: 'react-icons/' |
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.
Nitpick: We don't suffix our other build directories with -icons
(since they all are icons, by definition 😉). So let's simply call it react
or jsx
.
The idea is that we want to prepare Gridicons to be able to distribute both CommonJS and ECMAScript Modules. Initially, the idea was to use the directory root to publish the CJS modules so others could: import external from 'gridicons/external'; The main disadvantage of that approach is that, in the future, when ECMAScript modules become more common that namespaced will be taken by the CommonJS modules and we won't be able to change that if we want to be backwards-compatible. So, instead, the approach we're taking is being agnostic about what module system the library user wants to use by default. So, either: import external from 'gridicons/cjs/external'; import external from 'gridicons/esm/external'; will work.
package.json
Outdated
"main": "cjs/index.js", | ||
"files": [ | ||
"cjs" | ||
], |
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.
Indentation 😁
I'd include a slash (cjs/
) to make clear it's a directory (not just a file).
Can we also include esm/
?
@ockham addressed your feedback. Let me know if we are ready for a new npm release and I'll update the Calypso PR. |
This is looking awesome! Great changes -- I love that we now have template files instead of header and footer ones. One note, I'm seeing this pattern a lot in the generated const iconClass = [
'gridicon',
'gridicons-align-center',
className,
false ,
false,
isModulo18( size ) ? 'needs-offset-y' : false,
].filter( Boolean ).join( ' ' ); Think it's worth filtering the hard-coded |
Ah, that's something I've noticed as well. Tried to change it but made the code less readable (I like that the template is explicit about when an icon needs offset). Will leave this open until Automattic/wp-calypso#23171 is approved as well. Then, will change the gridicon package version in both before merging. |
In previous commits, I bumped to 3.0.1-alpha and 3.0.2-alpha instead of 3.0.0-alpha.1 and 3.0.0-alpha.2. I'm sorry! Taking advantage of the npm unpublish feature I've reverted those wrong alpha publications so I hope that, when you read this, they're available to you.
Fantastic work @nosolosw! Thanks so much for taking this on. ❤️ I did some smoke testing in Automattic/wp-calypso#23171 and things look pretty solid. Let's wait for a final sign off from flow (they're great at breaking stuff ✨), and try to land this next early next week. As an aside @folletto @jasmussen, do you think we could eventually move over social icons into this repo now that we can import a single icon at a time? social-logos has a very similar build and tends to lag behind gridicons. |
I'm not sure I understand why: the reason they are split isn't technical. These icon sets are based on entirely different principles and especially logos are regulated by specific copyright laws. I wouldn't merge the two unless there's a very compelling reason and a solution that covers all the concerns that separated them. I would work in making the process of aligning the build systems easier — as we did on so far. |
CHANGELOG.md
Outdated
* Package: added a `esnext` key in the package.json so API consumers can use it for importing the main ESM file. | ||
* Build: use template literals to create the React components and centralize the _icon needs offset_ logic in the svg-to-react Grunt task. | ||
* Build: deleted the `build/` directory in favor of separate `cjs/` and `esm/` folders that contain CommonJS and ECMAScript modules respectively. | ||
|
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.
Can we avoid acronyms (cjs, esm) in favor of a clear, spelled out folder name?
common-js
ecmascript
Also: can we add somewhere a detail on why there's this split, and as a developer when should I pick one or the other?
In genereal: is there any way to avoid this split for a simpler solution that reduces decision fatigue?
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.
That's a very good question I myself pondered about.
We can have a default import GridiconExternal from 'gridicons/external'
that pulls the CommonJS module (which is now the common format used in npm packages). That was actually the original proposal, but it would compromise future backward compatibility, as it was raised by @ockham Some notes on this:
- We live in times of change when it comes to JavaScript module formats. Many libraries are shipping both
cjs
andesm
. There is no a strong standard on how to do it yet. I wish we could just avoid this module war, but we can't. - If we publish now the
cjs
icons undergridicons
, whenesm
becomes more used we won't be able to use that namespace and maintain backward compatibility. We'll need to publish the individual icons undergridicons/esm
, for example. This would put us in a weird position and could potentially introduce errors if the consumer is expecting aesm
module when usinggridicons/external
, for example. - In the README, the
esm
option is deemphasized, which was intentional to mitigate decision fatigue. - Consumers of the API can always teach their module bundlers to make
gridicons
an alias forgridicons/cjs
orgridicons/esm
- this is probably what we'll do in Calypso.
Does this address your questions @folletto ?
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.
Yep, this is totally on me @folletto 😄
@nosolosw summed it up excellently. I'll just add that having untranspiled code available in esm/
allows the consuming project to make use of features such as tree-shaking. I.e. with some further iterations, we'll be able to do handy named imports such as
import { GridiconExternal, GridiconGarb } from 'gridicons';
while still ensuring that only the code that's relevant for these imports is being pulled in, thanks to tree-shaking -- i.e. we can get both nice named imports, and smaller bundle sizes. This is something we can't do with CommonJS.
(For reference, my original suggestion was at #283 (comment))
The different (CJS vs ESM) entrypoints are exposed through the main
and esnext
fields in package.json
; the general idea is taken from http://2ality.com/2017/06/pkg-esnext.html (and as Andrés said, we can use e.g. webpack aliases to still allow 'pretty' import directory filenames, e.g. without the explicit cjs/
or esm/
parts).
I've also seen some (albeit not a lot) of standardization towards these names ( cjs/
or esm/
), so I'd rather keep them. I'd like to think of them as an implementation detail that typically is aliased by the consumer anyway.
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.
Thank you for the reply. The technical answer is grounding for the question I have. I'm concerned on how cryptic now this is. Before "react/build" was clear. Now there's choice, with two three-letter shorthands that require knowing all of what you described to make a choice.
Granted, it's going to be packaged and published on npm, but is there any way to make this more understandable also in this library?
Also: I'm not sure if this is the case but when I read "it would compromise future backward compatibility" are we talking about Calypso or...? If Calypso already works with the more modern format, I'd suggest to ship just that and bump the requirements if needed. Or, at least, create a strong default for that, and leave the other as a secondary option.
At the very very least: can we pick a default, and have a one line explanation on when to use each of these two? Imagine a developer coming in and having no idea on the difference between the two when you write these one liners.
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.
'Default' now depends on context: If you project is CommonJS/ES5 only, use cjs/
. If it supports ES6 stuff -- most notably modules -- either natively, or thru transpilation, use esm/
. This is somwhat exposed by the relevant fields in package.json
, but I guess we can also document in the README
. Sounds good?
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.
But wouldn't that require moving the individual ESM files to the package root (or an alias on the consumer side) as well?
@ockham Good point, that was a mistake on my side. Both CJS and ES subpackages require moving files to package root. I edited my comment accordingly.
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.
@nosolosw WFM!
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.
In what cases is it required to specify the sideEffects flag to actually get tree-shaking?)
In case there is a top-level index.js
that reexports submodules:
export { default as GridiconNotice } from './notice';
export { default as GridiconSearch } from './search';
If we import only GridiconNotice
, how does Webpack and UglifyJS know they can omit the GridiconSearch
import? They don't, because importing the ./search
module can have side effects. For example, it can change some global variable or register a DOM listener.
The sideEffect: false
flag says that no such side effects exist and unused imports can be safely removed.
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.
@nosolosw Exporting just the CJS modules and forgetting about ES6 now works for me. The syntax would be:
import Gridicon from 'gridicons';
import GridiconExternal from 'gridicons/dist/external';
In future, when Webpack 4 becomes ubiquitous, we can migrate to full ES6:
import { GridiconExternal } from 'gridicons';
In the meantime, we could have a Babel transform that lets us write the named import syntax right now and transform it to a gridicons/dist/${icon}
import. We have something like this for Lodash and Redux Form already in Calypso. But that's optional.
I hope I summarized everything correctly :)
Summing up several threads and private convos with @folletto , @ockham and @jsnajdr this is the agreement:
The problem is that it doesn't allow us to reduce size with the majority of tooling, which was the goal of this PR. So we'll provide an escape hatch which is backward compatible:
I'll look into adding |
Let us know when you're ready for another look! |
Gruntfile.js
Outdated
@@ -143,7 +147,7 @@ module.exports = function( grunt ) { | |||
cwd: 'svg-min-react/', | |||
src: [ '**/*.svg' ], | |||
filter: 'isFile', | |||
dest: 'build/' | |||
dest: 'dist/' |
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.
I'm fine with moving the (CJS) stuff we're going to publish to dist/
; however, what about still building the intermediary ES6 files (that, IIUC, we aren't publishing for now) in build/
? Just to keep things a bit more separate -- the dist/
dir will then only hold stuff that we're also going to publish. That will allow us to set "files": [ "dist/" ]
instead of "files": [ "dist/*js" ]
in package.json
. (IMO, the latter is both uglier, and more fragile, since we're relying on *.js
(CJS) vs *.jsx
(ES6) extensions, which is quite subtle and easy to miss.)
Released 3.0.0-alpha.2 to npm without the named imports part - that is left for the future. Companion Calypso PR Automattic/wp-calypso#23171 was updated as well. This is ready for review and, hopefully, land! |
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.
One final request on my part.
Other than that, LGTM! Thanks a lot for your initiative and persistence! Awesome job!
@ockham Done at b884c31. Merged this PR and published to npm Thanks everyone for participating and for welcoming a newbie to the gridicons repo! I know this has taken longer than expected, but the topic is complex and I appreciate the effort everyone has done to understand each other until we've built a shared context and consensus. ❤️ Now, let's profit from this size reduction everywhere. |
Thank you for your openness to discussion and motivation to push this through, you're 🌟 |
Awesome work @nosolosw ! |
The goal of this PR is to prepare Gridicons to be imported by component.
To use the external Gridicon at the moment we do:
After this PR lands, with the new API we could also import the individual component directly:
By doing so, we're able to ship only the SVGs that the user needs and the size impact of using the Gridicons library in any project would become as minimal as it can be. For example, at the moment, to use the external Gridicon (278 bytes) in any project you'll pull 81K because the
Gridicon
component contains the whole icon set. By importing the individual component, you'll only pull 1,2K.Although this introduces the ability to import Gridicons individually, it doesn't remove the current way of importing the whole iconset at once.
How to test
build/
directory, if necessary.npm run build
.build/
directory containing untranspiled files exists.dist/
directory containing files transpiled to ES5 and CommonJS module format exists.npm pack
and verify that thegridicons-2.1.3.tgz
contains:dist
dir with the untranspiled filespackage.json
CHANGELOG.md
LICENSE.md
README.md