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

Remove startDateId and endDateId default prop values #866

Merged
merged 1 commit into from
Nov 30, 2017

Conversation

majapw
Copy link
Collaborator

@majapw majapw commented Nov 29, 2017

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

@majapw majapw added the semver-major: breaking change A non-backwards-compatible change; anything that breaks code - including adding a peerDep. label Nov 29, 2017
@coveralls
Copy link

Coverage Status

Coverage remained the same at 86.571% when pulling 1789935 on maja-make-drp-ids-required into e0d96f6 on master.

@ljharb
Copy link
Member

ljharb commented Nov 29, 2017

@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.

@majapw
Copy link
Collaborator Author

majapw commented Nov 29, 2017

@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.

@ljharb
Copy link
Member

ljharb commented Nov 29, 2017

Hmm, I guess that's technically breaking, but I also think people ignoring propType warnings are OK to break :-p

@majapw majapw added semver-patch: fixes/refactors/etc Anything that's not major or minor. and removed semver-major: breaking change A non-backwards-compatible change; anything that breaks code - including adding a peerDep. labels Nov 30, 2017
@majapw majapw merged commit 75a4341 into master Nov 30, 2017
@majapw majapw deleted the maja-make-drp-ids-required branch November 30, 2017 20:12
@JcBernack
Copy link

None of the documentation is updated to reflect this change, it still says "required: no". Also I can't find documentation what startDateId and endDateId actually even do or why I would want to set them. In the readme they are listed under OPTIONAL props (note the caps).

but because default props were also provided, then people wouldn't see an error when not using their own ids

Noone was ignoring an error, because there never was one. Removing the defaultProp absolutely was a breaking change. /rant 😉

@erin-doyle
Copy link
Collaborator

It looks like we have a few propType warnings when running the tests now. Not a big deal but kind of annoying:

Warning: Failed prop type: The prop `startDateId` is marked as required in `withStyles(DateRangePicker)`, but its value is `undefined`.
    in withStyles(DateRangePicker)
Warning: Failed prop type: The prop `endDateId` is marked as required in `withStyles(DateRangePicker)`, but its value is `undefined`.
    in withStyles(DateRangePicker)
Warning: Failed prop type: The prop `startDateId` is marked as required in `DateRangePicker`, but its value is `undefined`.
    in DateRangePicker
Warning: Failed prop type: The prop `endDateId` is marked as required in `DateRangePicker`, but its value is `undefined`.
    in DateRangePicker

@ljharb
Copy link
Member

ljharb commented Dec 15, 2017

Fair enough, my bad.

@majapw, should we revert this and publish a v16.0.2, and then rerevert it for a v17?

@majapw
Copy link
Collaborator Author

majapw commented Dec 15, 2017 via email

@ljharb
Copy link
Member

ljharb commented Dec 15, 2017

No prob, I'll take care of it today (revert + publish + put up rerevert PR)

@ljharb
Copy link
Member

ljharb commented Dec 16, 2017

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).

ljharb added a commit that referenced this pull request Dec 16, 2017
This reverts commit 75a4341, reversing
changes made to f09d924.

Turns out this was breaking.
@ljharb
Copy link
Member

ljharb commented Dec 16, 2017

v15.5.2 v15.5.3 is published; i'll follow up with a docs PR for v16 when I can.

majapw pushed a commit to aporlando/react-dates that referenced this pull request Apr 12, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver-patch: fixes/refactors/etc Anything that's not major or minor.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants