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

Default props to empty list #66

Closed
wants to merge 1 commit into from
Closed

Default props to empty list #66

wants to merge 1 commit into from

Conversation

zepfietje
Copy link

Status

READY

Breaking Changes

NO

Description

Defaulting props to an empty list takes away some boilerplate.
Both null and const [] were options, but I'm not sure if either of those has advantages over the other.

@zepfietje zepfietje requested a review from felangel as a code owner May 5, 2020 20:13
@felangel
Copy link
Owner

Hi @zepfietje 👋
Thanks for opening an PR 👍

For context, I intentionally didn't provide a default for props because I wanted to force developers to override it so they don't forget. One common problem with the previous implementation was it was extremely easy to forget to override props and then developers would be confused about why all instances were being considered equal. Thoughts?

@felangel felangel added the waiting for response Waiting for follow up label May 10, 2020
@felangel felangel added this to In progress in equatable May 10, 2020
@zepfietje
Copy link
Author

@felangel, that's exactly what I've been thinking about too. However, there's basically two steps in using Equatable: extending the equatable class and overriding the props. Therefore, I think it shouldn't be an issue if these 'installation' steps are documented clearly. I completely understand your point, but having to override props to be an empty list in e.g. every bloc state and event (abstract) class feels like a bigger flaw to me.

@felangel
Copy link
Owner

@jorgecoca thoughts on this?

@zepfietje
Copy link
Author

@jorgecoca just a kind reminder to get your thoughts on this proposal. 😉

@jorgecoca
Copy link

My apologies @zepfietje for the delay!

I am with @felangel on this one: ideally, the language would support proper object comparison, so Equatable would not be needed; however, given where we are, I'd rather be explicit and provide an empty list than letting Equatable provide a default value for me, and then dealing with "unknown" issues; I agree it is tedious, but it is less error prone at the same time the way it is right now.

@zepfietje
Copy link
Author

I've still got mixed feelings about it. However, I do fully understand the potential issue.

Thanks for your thoughts anyway @felangel and @jorgecoca!

@zepfietje zepfietje closed this May 29, 2020
@felangel
Copy link
Owner

@zepfietje I do too haha but it was a huge problem initially when props did not need to explicitly be provided. So many people opened issues saying Equatable wasn't working 😞

@felangel felangel added enhancement New feature or request and removed waiting for response Waiting for follow up labels May 29, 2020
@felangel felangel moved this from In progress to Done in equatable May 29, 2020
@zepfietje
Copy link
Author

zepfietje commented May 29, 2020

@felangel yeah, trade-off between convenience for new devs vs. people who've worked with Equatable before. Maybe the new EquatableConfig could offer a solution? Not yet sure how though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
equatable
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

3 participants