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

Add React propTypes removal #36

Merged
merged 2 commits into from
Aug 29, 2018
Merged

Add React propTypes removal #36

merged 2 commits into from
Aug 29, 2018

Conversation

lencioni
Copy link
Contributor

I originally started by adding this plugin to react-dates, but it seems
like we are going to want this in a lot of places, so it makes sense to
put it into our preset instead.

react-dates/react-dates#1322

For the default options, I decided to go with "wrap" as the mode, since
we are likely to use this more frequently in packages that are published
and consumed by other packages and apps, which aren't very compatible
with the "remove" option.

In our apps where this is not a concern, we will want to override this
to use the "remove" option in production and likely disable this plugin
entirely in development for better build speeds.

@lencioni lencioni requested a review from ljharb August 29, 2018 19:40
README.md Outdated
}
```

To disable this transformation, use the `removePropTypes` option:
Copy link
Collaborator

Choose a reason for hiding this comment

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

by making this default to true, it's a major change. i'd prefer to ship it with default false first, as a minor, and then switch to changing the default later.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ljharb I added a commit to make this disabled by default.

I originally started by adding this plugin to react-dates, but it seems
like we are going to want this in a lot of places, so it makes sense to
put it into our preset instead.

react-dates/react-dates#1322

For the default options, I decided to go with "wrap" as the mode, since
we are likely to use this more frequently in packages that are published
and consumed by other packages and apps, which aren't very compatible
with the "remove" option.

In our apps where this is not a concern, we will want to override this
to use the "remove" option in production and likely disable this plugin
entirely in development for better build speeds.
Enabling this by default will be a breaking change, so we want to ship
this in a non-breaking way at first and then we can later enable it by
default if we want to.
Copy link
Collaborator

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

LGTM, assuming you've validated that this works via npm link

@lencioni
Copy link
Contributor Author

@ljharb I've tested this using npm link in my local copy of react-dates, using the default (disabled), true, and also overriding the mode option and the additionalLibraries option. I think we are good to go!

@lencioni lencioni merged commit 7e57ca8 into master Aug 29, 2018
@lencioni lencioni deleted the proptypes branch August 29, 2018 20:01
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