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

Size limit support #691

Open
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

JordanForeman
Copy link

@JordanForeman JordanForeman commented Apr 26, 2022

What Changed

fixes #690

Adds a new option for ds size that enables enforcement of a limit on component bundles. Refactors the size plugin significantly in order to support locally-scoped configuration options, namely, the sizeLimit option.

Why

I'm building a new Design System component and want to enforce a limit on the component as a whole, rather than simply on incoming changesets

Todo:

  • Add tests
    • Tests don't exist for this already, so it's unclear to me how/if to try to add any
  • Add docs
    • Looks like docs all exist in the gh-pages branch. What's the process for adding to that? Just a separate PR to gh-pages?
📦 Published PR as canary version: 4.14.1--canary.691.19342.0

✨ Test out this PR locally via:

npm install @design-systems/babel-plugin-include-styles@4.14.1--canary.691.19342.0
npm install @design-systems/babel-plugin-replace-styles@4.14.1--canary.691.19342.0
npm install @design-systems/cli-utils@4.14.1--canary.691.19342.0
npm install @design-systems/cli@4.14.1--canary.691.19342.0
npm install @design-systems/core@4.14.1--canary.691.19342.0
npm install @design-systems/create@4.14.1--canary.691.19342.0
npm install @design-systems/docs@4.14.1--canary.691.19342.0
npm install @design-systems/eslint-config@4.14.1--canary.691.19342.0
npm install @design-systems/hooks@4.14.1--canary.691.19342.0
npm install @design-systems/load-config@4.14.1--canary.691.19342.0
npm install @design-systems/next-esm-css@4.14.1--canary.691.19342.0
npm install @design-systems/plugin@4.14.1--canary.691.19342.0
npm install @design-systems/stylelint-config@4.14.1--canary.691.19342.0
npm install @design-systems/svg-icon-builder@4.14.1--canary.691.19342.0
npm install @design-systems/utils@4.14.1--canary.691.19342.0
npm install @design-systems/build@4.14.1--canary.691.19342.0
npm install @design-systems/bundle@4.14.1--canary.691.19342.0
npm install @design-systems/clean@4.14.1--canary.691.19342.0
npm install @design-systems/create-command@4.14.1--canary.691.19342.0
npm install @design-systems/dev@4.14.1--canary.691.19342.0
npm install @design-systems/lint@4.14.1--canary.691.19342.0
npm install @design-systems/playroom@4.14.1--canary.691.19342.0
npm install @design-systems/proof@4.14.1--canary.691.19342.0
npm install @design-systems/size@4.14.1--canary.691.19342.0
npm install @design-systems/storybook@4.14.1--canary.691.19342.0
npm install @design-systems/test@4.14.1--canary.691.19342.0
npm install @design-systems/update@4.14.1--canary.691.19342.0
# or 
yarn add @design-systems/babel-plugin-include-styles@4.14.1--canary.691.19342.0
yarn add @design-systems/babel-plugin-replace-styles@4.14.1--canary.691.19342.0
yarn add @design-systems/cli-utils@4.14.1--canary.691.19342.0
yarn add @design-systems/cli@4.14.1--canary.691.19342.0
yarn add @design-systems/core@4.14.1--canary.691.19342.0
yarn add @design-systems/create@4.14.1--canary.691.19342.0
yarn add @design-systems/docs@4.14.1--canary.691.19342.0
yarn add @design-systems/eslint-config@4.14.1--canary.691.19342.0
yarn add @design-systems/hooks@4.14.1--canary.691.19342.0
yarn add @design-systems/load-config@4.14.1--canary.691.19342.0
yarn add @design-systems/next-esm-css@4.14.1--canary.691.19342.0
yarn add @design-systems/plugin@4.14.1--canary.691.19342.0
yarn add @design-systems/stylelint-config@4.14.1--canary.691.19342.0
yarn add @design-systems/svg-icon-builder@4.14.1--canary.691.19342.0
yarn add @design-systems/utils@4.14.1--canary.691.19342.0
yarn add @design-systems/build@4.14.1--canary.691.19342.0
yarn add @design-systems/bundle@4.14.1--canary.691.19342.0
yarn add @design-systems/clean@4.14.1--canary.691.19342.0
yarn add @design-systems/create-command@4.14.1--canary.691.19342.0
yarn add @design-systems/dev@4.14.1--canary.691.19342.0
yarn add @design-systems/lint@4.14.1--canary.691.19342.0
yarn add @design-systems/playroom@4.14.1--canary.691.19342.0
yarn add @design-systems/proof@4.14.1--canary.691.19342.0
yarn add @design-systems/size@4.14.1--canary.691.19342.0
yarn add @design-systems/storybook@4.14.1--canary.691.19342.0
yarn add @design-systems/test@4.14.1--canary.691.19342.0
yarn add @design-systems/update@4.14.1--canary.691.19342.0

@adierkens
Copy link
Collaborator

Looks like docs all exist in the gh-pages branch. What's the process for adding to that?

The docs are all located here and get published to gh-pages upon release.

Per #690

It would be ideal to be able to specify a size limit on a component-by-component basis. I'm super new to this codebase, but it seems like ds.config is system-wide. If it's possible to make component-specific configurations, then exposing a size.limit configuration for a given component that is enforced by ds size would be great. Barring that, setting a size.limit for all components should be feasible as well.

IMO, I don't think a system-wide size-limit would really provide a whole lot of value. This is one of the reasons we went with a % increase approach since that would take into account the baseline complexities of each component. With 1 global upper-bound, components like a Link and Carousel would both end up sharing the same budget.

I think a per-component size limit would be right way to go about this.

@JordanForeman
Copy link
Author

@adierkens Updated to support package-specific size limits! It still supports global limits, but yeah I don't see that being used often. Feel free to re-review.

In order to support package-local configuration I needed to refactor the size command quite a bit. I tried to break this refactor up over a few descriptive commits, so it might be easier to review by commit.

cc. @yucho

@@ -1,4 +1,5 @@
import { monorepoName, createLogger } from '@design-systems/cli-utils';
import { cosmiconfigSync as load } from 'cosmiconfig';
Copy link
Author

Choose a reason for hiding this comment

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

💡 would prefer to use @design-systems/load-config here, but trying to link that resulted in a weird circular dependency issue that broke installation (see previous build). Got tired of fighting with that, but if someone else wants to tackle that optimization then by all means.

@kelyvin kelyvin added the minor Increment the minor version when merged label Jun 14, 2022
@JordanForeman
Copy link
Author

Thanks for the approval @kelyvin! Can you hold off on merging for the time being? I have been meaning to attach a demo to this PR, and IIRC I was having difficulty that suggested this feature wasn't working. I need to carve out some time to revisit this. I'll nudge you when this is ready.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
minor Increment the minor version when merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for component size limits
4 participants