Skip to content

Create actual browser bundles and transpile everything else#789

Merged
BinaryMuse merged 30 commits intorelease-19.0.0from
mkt/bundle-up
May 7, 2020
Merged

Create actual browser bundles and transpile everything else#789
BinaryMuse merged 30 commits intorelease-19.0.0from
mkt/bundle-up

Conversation

@BinaryMuse
Copy link
Copy Markdown
Contributor

@BinaryMuse BinaryMuse commented Apr 29, 2020

Required reading: #779

This PR significantly changes up our bundling situation:

  • Rollup now produces two actual bundles: dist/browser.esm.js and dist/browser.umd.js. These are actual browser bundles — all dependencies except for our peer dependencies (react and styled-components) are included in the bundle. These are really only useful for dropping onto a page with the appropriate ESM/UMD setup (e.g. the user is also including the peer deps). We may decide we don't even need these, but they may be useful for tracking deltas in size over time. Note: these files are quite large (500KB, even after minification), but include all the dependencies.
  • Everything else gets transpiled directly from src/ into dist/. This will allow users to use both import {Box, BorderBox} from '@primer/components' and also import Box from '@primer/components/dist/Box'. This also enables import ShinyNewThing from '@primer/components/dist/experimental/ShinyNewThing'

Todo:

Release Notes

  • You can now import individual components without having to include the entire package or set up tree-shaking; for example, import {Box} from '@primer/components' can now also be written as import Box from '@primer/components/dist/Box'

Closes #779
Closes #456
Closes #797
Closes #801

@vercel
Copy link
Copy Markdown

vercel bot commented Apr 29, 2020

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/primer/primer-components/ni2ajrvgh
✅ Preview: https://primer-components-git-mkt-bundle-up.primer.now.sh

@vercel vercel bot temporarily deployed to Preview April 29, 2020 18:31 Inactive
@vercel vercel bot temporarily deployed to Preview April 29, 2020 19:34 Inactive
@vercel vercel bot temporarily deployed to Preview April 29, 2020 19:46 Inactive
@BinaryMuse
Copy link
Copy Markdown
Contributor Author

BinaryMuse commented Apr 29, 2020

Since this PR will make every component importable directly from its transpiled source file, each component's exports shape will be part of the public API of the package. As such, I'm proposing making that contract explicit in our tests:

checkExports('Position', {
  default: null,
  Position,
  Absolute,
  Fixed,
  Relative,
  Sticky
})

checkExports('Box', {
  default: Box
})

checkExports will ensure that the file at the given path exports the things we expect; if there are keys in the expected exports that don't match, or there are things exported that we don't expect to be exported, the test will fail.

Before I go through the entire codebase adding this, I wanted to get some feedback from the team to see how people felt about such a thing.

@BinaryMuse BinaryMuse mentioned this pull request Apr 29, 2020
24 tasks
@vercel vercel bot temporarily deployed to Preview April 29, 2020 23:53 Inactive
@vercel vercel bot temporarily deployed to Preview April 29, 2020 23:56 Inactive
@vercel vercel bot temporarily deployed to Preview April 30, 2020 19:24 Inactive
@vercel vercel bot temporarily deployed to Preview April 30, 2020 19:27 Inactive
As we begin to implement things like deprecation warnings that we only
want to appear in non-production environments, it's useful to have an
easy check that completely compiles out in production. This commit
adds `babel-plugin-transform-replace-expressions`, which replaces
expressions in code.

The primary expression is `__DEV__`, which compiles to
`process.env.NODE_ENV !== "production"`. When building the browser
bundles, we compile it instead to `false`, so that terser automatcally
removes the entire `if(__DEV__)...` block, resulting in zero runtime
overhead.

This commit also adds the define to Gatsby's webpack config, as Gatsby
doesn't seem to respect the Babel configuration for the main Primer
Components project. In addition, to get Gatsby to run Primer Components
through the new webpack plugin we add, we need to alter the webpack
rules to include files loaded from `@primer/components`.

Finally, when building a production build (like in our browser bundles),
we explicitly set `process.env.NODE_ENV` to `"production"`.
@vercel vercel bot temporarily deployed to Preview April 30, 2020 19:28 Inactive
@vercel vercel bot temporarily deployed to Preview April 30, 2020 20:20 Inactive
@vercel vercel bot temporarily deployed to Preview April 30, 2020 21:10 Inactive
@BinaryMuse
Copy link
Copy Markdown
Contributor Author

BinaryMuse commented Apr 30, 2020

As I was looking through the exports, I noticed two modules where importing directly from the dist folder was a little weird: the Button variants and the Position variants. I've modified them as such:

  • Button and variants can now be imported as:

    import Button, {
      ButtonDanger,
      ButtonPrimary,
      ButtonOutline,
      ButtonGroup,
      ButtonTableList
    } from '@primer/components/dist/Button'
  • Position and variants can now be imported as:

    import Position, {
      Absolute,
      Fixed,
      Sticky,
      Relative
    } from '@primer/components/dist/Position'

Open to other thoughts on organization, and also curious the best way to go about documenting individual exports when the component isn't directly available on the default export.

@BinaryMuse
Copy link
Copy Markdown
Contributor Author

With some more changes it's sitting at ~170KB.

image

@vercel vercel bot temporarily deployed to Preview May 4, 2020 21:33 Inactive
@vercel vercel bot temporarily deployed to Preview May 4, 2020 21:48 Inactive
@vercel vercel bot temporarily deployed to Preview May 4, 2020 22:42 Inactive
@vercel vercel bot temporarily deployed to Preview May 4, 2020 22:53 Inactive
Deprecation testing and zero runtime cost in prod
@vercel vercel bot temporarily deployed to Preview May 4, 2020 22:56 Inactive
@BinaryMuse BinaryMuse mentioned this pull request May 5, 2020
8 tasks
@emplums
Copy link
Copy Markdown

emplums commented May 5, 2020

I like the checkExports idea!

Copy link
Copy Markdown

@emplums emplums left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great! might be helpful to have at least another person review this too since it's lots of changes!

Copy link
Copy Markdown
Contributor

@dmarcey dmarcey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm definitely not a rollup / babel expert, but I looked through the changes to try to give another set of 👀 and things looked good to me! Nice work! ✨

@T-Hugs
Copy link
Copy Markdown
Contributor

T-Hugs commented May 6, 2020

Potentially a dumb question: Is there a real benefit to shipping bundles in the first place? How many consumers want a UMD module bundle? If the answer is "not really" could we just get rid of rollup completely and force consumers to use a bundler (because let's be honest, if you're writing frontend JS you're using a bundler).

@BinaryMuse
Copy link
Copy Markdown
Contributor Author

Is there a real benefit to shipping bundles in the first place?

I don't think so, and in fact it's not documented. I think it's potentially useful for us internally for seeing how our package would behave if it was bundled; a good example is cutting off ~800KB off the minified library size based off the work in this PR.

@vercel vercel bot temporarily deployed to Preview May 7, 2020 00:04 Inactive
@BinaryMuse BinaryMuse merged commit 9e8536e into release-19.0.0 May 7, 2020
@colebemis colebemis deleted the mkt/bundle-up branch October 29, 2021 00:44
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