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

Deprecation testing and zero runtime cost in prod #791

Merged
merged 3 commits into from May 4, 2020

Conversation

BinaryMuse
Copy link
Contributor

This PR builds on the Babel and compilation changes in #789 and @emplums' deprecation work in #777. It adds the following features:

  • a useDeprecation hook that ensures we only log a deprecation warning to the console once if a component re-renders. For example:

    const Flash = ({variant, scheme, ...props}) => {
      const deprecate = useDeprecation({
        name: 'Flash "scheme" prop',
        version: '20.0.0',
        message: 'Use the variant prop instead. See https://primer.style/components/Flash for more details.'
      })
    
      if (scheme) {
        deprecate()
        variant = schemeMap[scheme]
      }
    
      return <StyledFlash variant={variant} {...props} />
    }
  • the ability to test for deprecations, e.g.:

    it('respects the deprecated "scheme" prop', () => {
      expect(render(<Flash scheme="green" theme={theme} />)).toHaveStyleRule('background-color', colors.green[1])
      expect(Deprecations.getDeprecations()).toHaveLength(1)
    })
  • a global test to ensure we're not shipping deprecated code that we meant to be removed:

    import semver from 'semver'
    import {Deprecations} from '../utils/deprecate'
    
    const ourVersion = require('../../package.json').version
    
    beforeEach(() => {
      Deprecations.clearDeprecations()
    })
    
    afterEach(() => {
      const deprecations = Deprecations.getDeprecations()
    
      for (const dep of deprecations) {
        if (semver.gte(ourVersion, dep.version)) {
          throw new Error(`Found a deprecation that should be removed in ${dep.version}`)
        }
      }
    })
  • zero runtime overhead in production

    All the deprecation code is wrapped in if (__DEV__), which gets compiled to if (process.env.NODE_ENV !== "production"). When a user bundles their code and sets NODE_ENV to "production", this changes to if (false) and minifiers will remove the entire function block

@vercel
Copy link

vercel bot commented May 1, 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/hlfazcsks
✅ Preview: https://primer-components-git-mkt-deprecation-testing.primer.now.sh

@BinaryMuse BinaryMuse requested a review from emplums May 1, 2020 23:42
@BinaryMuse
Copy link
Contributor Author

Since each deprecation has a name property, we could also modify the system to only warn once per deprecation type instead of deprecation instance. However, it may be useful to be notified if you're using the same deprecated code in multiple places in your codebase. Grim reports once per source code location by generating and storing some stack frames.

Copy link

@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! Thanks for running with this! 🙌 Do we need to add any docs anywhere about setting NODE_ENV?

@BinaryMuse
Copy link
Contributor Author

@emplums We currently don't have any docs relating to using Primer Components with a module bundler. It's standard practice (at least in React apps) to compile using NODE_ENV="production" because it pulls out a ton of dev-mode stuff from React, but we could always add a doc on bundling.

@emplums
Copy link

emplums commented May 4, 2020

@BinaryMuse I think a small note in Getting Started just saying something like "If you'd like deprecation warnings not to appear in production, make sure you've got NODE_ENV="production" set in your bundler.

@BinaryMuse BinaryMuse mentioned this pull request May 4, 2020
@vercel vercel bot temporarily deployed to Preview May 4, 2020 22:26 Inactive
@BinaryMuse
Copy link
Contributor Author

Merging this down to the base PR

@BinaryMuse BinaryMuse merged commit 094df0a into mkt/bundle-up May 4, 2020
@BinaryMuse BinaryMuse deleted the mkt/deprecation-testing branch May 4, 2020 22:56
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

2 participants