primer / react

An implementation of GitHub's Primer Design System using React
https://primer.style/guides/react
MIT License
3.04k stars 527 forks source link

[Discussion] Bundle disambiguation #779

Closed BinaryMuse closed 4 years ago

BinaryMuse commented 4 years ago

As I've begun to look into our bundling situation more closely, I'm starting to think that maybe we've conflated a few terms over time, and that we may not all talking about the same thing. In this issue I'll lay out where we stand now, propose some terminology for future discussions, and some options on where we want to go from here.

Bundles as browser targets

When I hear the term "bundle," I think of browser bundles: a bunch of JS that has been rolled up into one (or a few) files, together with all its dependencies, in order to be dropped in a page and run in the browser. These bundles are created by "module bundlers," such as webpack and rollup.

Rollup is a module bundler for JavaScript

webpack is a module bundler. Its main purpose is to bundle JavaScript files for usage in a browser...

In the case of Primer Components, that means that a user could take our dist/index.umd.js file, drop it into any UMD environment (like a browser page), and with no dependencies except for the ones we mark as external — currently only styled-components and react — get up and running using Primer Components.

Looking at our rollup config, it looks like that's what we were aiming to do. However, what we produce with rollup is not a browser bundle. If you take a look at the output while bundling, you'll see all the dependencies that aren't being included in the bundle:

$ yarn dist
yarn run v1.22.4
$ rm -rf dist
$ NODE_ENV=production rollup -c

src/index.js → dist/index.esm.js, dist/index.umd.js...
Browserslist: caniuse-lite is outdated. Please run next command `yarn upgrade`
(!) Unresolved dependencies
https://rollupjs.org/guide/en/#warning-treating-module-as-external-dependency
@primer/primitives (imported by src/theme.js)
polished (imported by src/theme.js)
prop-types (imported by src/BaseStyles.js, src/BorderBox.js, src/Box.js, src/Position.js, src/AvatarPair.js, src/Avatar.js, src/AvatarStack.js, src/BranchName.js, src/Breadcrumbs.js, src/Button.js, src/Caret.js, src/CircleBadge.js, src/CounterLabel.js, src/CircleOcticon.js, src/Details.js, src/Dialog.js, src/Dropdown.js, src/Flash.js, src/FilteredSearch.js, src/FilterList.js, src/Heading.js, src/Label.js, src/PointerBox.js, src/Link.js, src/Popover.js, src/ProgressBar.js, src/SideNav.js, src/StyledOcticon.js, src/StateLabel.js, src/TabNav.js, src/constants.js, src/Tooltip.js, src/Text.js, src/SubNav.js, src/TextInput.js, src/Timeline.js, src/UnderlineNav.js, src/Truncate.js, src/ButtonBase.js, src/Pagination/Pagination.js, src/SelectMenu/SelectMenu.js, src/SelectMenu/SelectMenuFilter.js, src/SelectMenu/SelectMenuItem.js, src/SelectMenu/SelectMenuTab.js, src/SelectMenu/SelectMenuTabPanel.js)
styled-system (imported by src/Avatar.js, src/Caret.js, src/Dialog.js, src/Label.js, src/Link.js, src/ProgressBar.js, src/constants.js, src/TextInput.js, src/Truncate.js, src/ButtonBase.js)
@styled-system/prop-types (imported by src/Avatar.js, src/Button.js, src/Dialog.js, src/ProgressBar.js, src/constants.js, src/TextInput.js, src/ButtonBase.js)
classnames (imported by src/Breadcrumbs.js, src/Popover.js, src/SideNav.js, src/TabNav.js, src/Tooltip.js, src/SubNav.js, src/TextInput.js, src/Timeline.js, src/UnderlineNav.js, src/SelectMenu/SelectMenuTab.js)
@primer/octicons-react (imported by src/CircleBadge.js, src/CircleOcticon.js, src/Dialog.js, src/StyledOcticon.js, src/StateLabel.js, src/SelectMenu/SelectMenuItem.js)
@reach/dialog (imported by src/Dialog.js)
nanoid (imported by src/FilterList.js)
@styled-system/theme-get (imported by src/constants.js)
@styled-system/props (imported by src/TextInput.js)
react-is (imported by src/utils/elementType.js)
(!) Missing global variable names
Use output.globals to specify browser global variable names corresponding to external modules
@primer/primitives (guessing 'primitives')
polished (guessing 'polished')
styled-system (guessing 'styledSystem')
prop-types (guessing 'PropTypes')
@styled-system/prop-types (guessing 'systemPropTypes')
@styled-system/theme-get (guessing 'themeGet')
react (guessing 'React')
styled-components (guessing 'styled')
classnames (guessing 'classnames')
@primer/octicons-react (guessing 'Octicon')
@reach/dialog (guessing 'dialog')
nanoid (guessing 'nanoid')
react-is (guessing 'reactIs')
@styled-system/props (guessing 'props')
created dist/index.esm.js, dist/index.umd.js in 2.8s

If you take a look at the URL specified in the output, it confirms the situation:

Rollup will only resolve relative module IDs by default. This means that an import statement like this…

import moment from 'moment';

…won't result in moment being included in your bundle – instead, it will be an external dependency that is required at runtime. If that's what you want, you can suppress this warning with the external option, which makes your intentions explicit:

// rollup.config.js
export default {
  entry: 'src/index.js',
  dest: 'bundle.js',
  format: 'cjs',
  external: [ 'moment' ] // <-- suppresses the warning
};

If you do want to include the module in your bundle, you need to tell Rollup how to find it. In most cases, this is a question of using @rollup/plugin-node-resolve.

Some modules, like events or util, are built in to Node.js. If you want to include those (for example, so that your bundle runs in the browser), you may need to include rollup-plugin-node-polyfills.

So if we're not creating a browser bundle, what are we creating?

Babel and transpilation

Ah, Babel, the tool we all love and hate. As you know, to support JSX, styled-components' css prop, macros, and other non-standard JavaScript syntax, we need to run it through Babel to turn it into regular JavaScript. Without such a thing, users of Primer Components would need to configure tooling to do all that Babel transpilation for them.

This is why users can't import Pagination from '@primer/components/src/Pagination' — that file is full of JSX, and the user's toolchain may not be set up to handle JSX. So, they instead import {Pagination} from '@primer/components', which dives into dist/index.umd.js and pulls out the necessary component from the ~bundle~ transpiled file (it's not really quite a bundle).

The downside to this, as we've been discussing, is that, without tree-shaking, such an import brings in the entirety of Primer Components — all ~140KB of it (although it's worth noting that we do no minification, nor do we even remove comments, so there's a lot of room for improvement here).

So what do we actually want?

We've been talking about "per-component bundles" as a mechanism for getting around including the entirety of Primer Components, allowing users to import a single component, e.g. import Pagination from '@primer/components/dist/Pagination'. However, by making these files bundles, we'd be including all our dependencies, a copy of the theme, and all other shared resources into every generated component bundle. (There are ways to avoid this by generating things like shared and vendor chunks which can get placed into pages separately.)

However, I think what we really want when we say "per-component bundles" is simply a transpiled version of the source file(s) that users can import individually without needing to have Babel set up in their projects. I think we should refer to such a thing as a "transpiled source file" or similar, to disambiguate it from an actual browser bundle that's ready to run in a UMD environment.

So now what

I think that laying out a list of the workflows we want to support will help us answer this question.

I think it's clear that we definitely want the first two items, but it's not so clear if the latter two are all that important to us. If they're not, it should allow us to completely drop rollup from our build process and simply transpile our files instead of building an (incomplete) browser bundle.

(Note: we could always keep creating browser bundles just for determining the approximate size of the project when included in a browser.)

BinaryMuse commented 4 years ago

Closed in #789