-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Remove startDateId and endDateId default prop values #866
Conversation
@majapw why is this breaking? If the props are required, then the defaultProp was never needed. We definitely shouldn't worry about someone who was ignoring propType warnings. |
@ljharb the props have been required for a while, but because default props were also provided, then people wouldn't see an error when not using their own ids. if we remove the default props, then people who were previously not seeing an error will now see one. that was my motivation for marking it as breaking. |
Hmm, I guess that's technically breaking, but I also think people ignoring propType warnings are OK to break :-p |
None of the documentation is updated to reflect this change, it still says "required: no". Also I can't find documentation what
Noone was ignoring an error, because there never was one. Removing the defaultProp absolutely was a breaking change. /rant 😉 |
It looks like we have a few propType warnings when running the tests now. Not a big deal but kind of annoying:
|
Fair enough, my bad. @majapw, should we revert this and publish a v16.0.2, and then rerevert it for a v17? |
That seems reasonable to me! It'd be good to add a blurb about the id as
well in the README.
I am sans computer for ten days so would appreciate if you did that @ljharb.
Sorry for the warnings!
…On Fri, Dec 15, 2017, 11:12 AM Jordan Harband ***@***.***> wrote:
Fair enough, my bad.
@majapw <https://github.com/majapw>, should we revert this and publish a
v16.0.2, and then rerevert it for a v17?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#866 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABUdtVoxxG4RHFLxwqOtKptOyPVgPJYDks5tAqiAgaJpZM4QuXjl>
.
|
No prob, I'll take care of it today (revert + publish + put up rerevert PR) |
Whoops, this is actually in v15. I'll revert it in v15.5.2 tonight/tomorrow, but leave it in v16 (and add the docs). |
|
Fix for #326
The ids were made required but their default prop values were not removed and as such the
required
aspect was useless. This addresses that issue.This is technically breaking.
to: @ljharb @erin-doyle @airbnb/webinfra