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

make button as check strip out completely in production builds #2666

Merged
merged 2 commits into from
Dec 8, 2022

Conversation

mattcosta7
Copy link
Collaborator

@mattcosta7 mattcosta7 commented Dec 8, 2022

Describe your changes here.

#2659 introduced a dev only check in an effect, but it doesn't completely strip that effect from production builds. This removes the entire effect from the production bundle by wrapping the whole effect in the compile flag check (which violates the eslint rule of hooks, but doesn't actually break the rules since this isn't conditional, but instead just included or excluded based on build properties - it will never change between render passes)

Screenshots

Please provide before/after screenshots for any visual changes

Merge checklist

No real behavior change should be apparent here

@changeset-bot
Copy link

changeset-bot bot commented Dec 8, 2022

🦋 Changeset detected

Latest commit: 961d92a

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@primer/react Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@github-actions
Copy link
Contributor

github-actions bot commented Dec 8, 2022

size-limit report 📦

Path Size
dist/browser.esm.js 82.77 KB (-0.04% 🔽)
dist/browser.umd.js 83.42 KB (-0.05% 🔽)

@mattcosta7 mattcosta7 temporarily deployed to github-pages December 8, 2022 17:04 — with GitHub Actions Inactive
@github-actions github-actions bot temporarily deployed to storybook-preview-2666 December 8, 2022 17:04 Inactive
@mattcosta7 mattcosta7 changed the title make as check strip out completely in production builds make button as check strip out completely in production builds Dec 8, 2022
React.useEffect(() => {
if (!(innerRef.current instanceof HTMLButtonElement) && !(innerRef.current instanceof HTMLAnchorElement)) {
if (__DEV__) {
if (__DEV__) {
Copy link
Contributor

Choose a reason for hiding this comment

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

thank you for the explanation!

@joshblack joshblack enabled auto-merge (squash) December 8, 2022 22:28
@joshblack joshblack temporarily deployed to github-pages December 8, 2022 22:33 — with GitHub Actions Inactive
@joshblack joshblack merged commit d995bb8 into main Dec 8, 2022
@joshblack joshblack deleted the compile-time-wrap-for-button-as-check branch December 8, 2022 22:34
@primer-css primer-css mentioned this pull request Dec 8, 2022
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.

None yet

3 participants