Skip to content

docs(adr): add development-warnings adr #2928

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.

Sign up for GitHub

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

Merged
joshblack merged 4 commits into from
Mar 1, 2023
Merged

docs(adr): add development-warnings adr #2928

Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
321801a
docs(adr): add development-warnings adr
joshblack Feb 22, 2023
9bec22e
Update adr-XXX-development-warnings.md
joshblack Feb 22, 2023
f6b47bc
chore: update false-y to truth-y for warning check
joshblack Feb 28, 2023
7e42365
chore: update title
joshblack Feb 28, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
60 changes: 60 additions & 0 deletions contributor-docs/adrs/adr-XXX-development-warnings.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,60 @@
# ADR XXX: Development Warnings

## Status

Proposed

## Context

There are situations where we would like to provide warnings to developers who
use `@primer/react` that something may be deprecated, unsupported, etc. Often,
these will be emitted using `console.warn()`. When using `console.warn()` by
itself, we run into a situation where code that is only meant for development
is included in production code. As a result, it would be helpful to establish
patterns around how to provide warnings to developers in order to make sure
that:

- Calls to `console.warn()` do not appear in production
- Code related to development checks, warnings, or messages are removed from
production code

## Decision

Code that is meant for development-only warnings or checks **must** be wrapped within a
`__DEV__` block.

```tsx
function ExampleComponent() {
if (__DEV__) {
// This code only runs in development
}
}
```

Under-the-hood, the `__DEV__` block will be compiled to a `NODE_ENV` check so
that it is stripped when `NODE_ENV` is set to `'production'`.

> **Note**
> Contributors may wrap hooks within a `__DEV__` block even though hooks are not
> meant to be called conditionally. This is because the `__DEV__` check can be
> considered constant in that it will always be true or false for the
> environment (development or production).

When a contributor would like to communicate a warning to a developer, they
should use the `warning()` helper.

```ts
warning(condition, 'This is the message that is logged when condition is false-y')

@colebemis colebemis Feb 22, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I would have guessed that the message would display if the condition is truthy. I read that code as "warn if condition"

If this is a convention, I'm fine to keep it. But I wanted to mention my initial confusion.

If the name of the helper was something like assert(), I think falsey would make more sense.

@broccolinisoup broccolinisoup Feb 24, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I would have guessed that the message would display if the condition is truthy. I read that code as "warn if condition"

Ohh, this is how I think it was as well the entire time - now realised that 🙈

@joshblack joshblack Feb 24, 2023

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Totally makes sense! Happy to flip it if that's more intuitive for folks 👍

I think the convention stems from invariant() ultimately, and things like assert(), that frame things from the perspective of "this condition must be met". When it's not met, throw an error, log a warning, etc

To contextualize the two, here is the warning from Heading in the two styles:

// Warn if condition is true
warning(
  innerRef.current && !(innerRef.current instanceof HTMLHeadingElement),
  'This Heading component should be an instanceof of h1-h6'
);
// Warn if condition is false
warning(
  innerRef.current && innerRef.current instanceof HTMLHeadingElement,
  'This Heading component should be an instanceof of h1-h6'
);

@broccolinisoup broccolinisoup Feb 26, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks Josh! Warning when the condition truthy feels more intuitive to me. That said, it is totally cool to me if we want follow the convention that it stems from.

@joshblack joshblack Feb 27, 2023

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Let's do truth-y then! I'll update the ADR accordingly 👍

```

This helper allows you to provide a `condition`. When the condition is false-y
it will emit the message provided. This helper is automatically wrapped in a
`__DEV__` block and will be removed from production builds.

For more complex conditions, a contributor may combine `console.warn()` with
`__DEV__` when `warning()` does not suffice.

### Impact

- Calls to `console.warn()` will be replaced by `warning()` or will be wrapped
in a `__DEV__` block