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

[WIP] Backward compatibility for the query prop #123

Merged
merged 2 commits into from
Mar 25, 2019

Conversation

edorivai
Copy link
Collaborator

@edorivai edorivai commented Mar 17, 2019

Tracking issue: #121.

This is a work in progress.

Todo:

Motivation

As per the comment in #119, this allows the use of both the query, as well as the queries prop.

The queries prop was introduced in #72, and first discussed in #69. It's been available in the next branch for a while now, and after giving it some thought, I think there is significant value in keeping the query prop in a backward-compatible manner:

  • It will allow us to publish all of the changes currently waiting in next under a minor version bump. The current changes include a bugfix for SSR, so it would be nice for people to be able to get these change in a minor version increment.
  • Accordingly, it will allow consumers to update to this new version without having to change their use of this library. Most notably, without this backward compatibility, one would be forced to rewrite all <Media query=...> occurences to <Media queries=...>. I can imagine this would be sufficiently painful for larger codebases that it will deter people from upgrading.

Discussion

Allowing both query as well as queries does add some complexity to both the implementation and the API. I'm not too worried about the implementation, but for the API it means that ideally, we would require either query or queries to be defined.

I've currently implemented this constraint manually, but I suppose this could be done in a bit of a cleaner manner using PropTypes. If anyone knows how to implement such a constraint properly, do let me know 🤓 (@tstirrat15 ?). Quick note, I'm aware of airbnb-prop-types, and think we'd probably need something like this. I'm just not comfortable with including that lib as a dependency here, it feels a bit overkill.

Next steps

My motivation of shipping all of the current changes under a minor version are described above. What I'd really like to do is to implement a v2 based off of hooks. In this v2, I'd suggest we implement the business logic with hooks, and additionally expose the old component API (<Media>) which would then use the hooks internally. The only actual reason for making such an update a major version bump would be because us using hooks would require consumers to ensure they have a version of React installed that ships with hooks. Read: we'd bump the React peerDependency.

Copy link
Contributor

@tstirrat15 tstirrat15 left a comment

Choose a reason for hiding this comment

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

I had a look through and it looks good to me.

@tstirrat15
Copy link
Contributor

tstirrat15 commented Mar 21, 2019

Regarding your point on queries XOR query, I don't currently know how to do that with PropTypes off the top of my head.

It looks like you could write a pair of custom validators where each prop checks the other to make sure that they're not both defined:

const propTypes = {
  // You can also specify a custom validator. It should return an Error
  // object if the validation fails. Don't `console.warn` or throw, as this
  // won't work inside `oneOfType`.
  customProp: function(props, propName, componentName) {
    if (!/matchme/.test(props[propName])) {
      return new Error(
        'Invalid prop `' + propName + '` supplied to' +
        ' `' + componentName + '`. Validation failed.'
      );
    }
  },
}

Which might be a bit cumbersome, but it looks like that'd have the correct behavior.

@edorivai
Copy link
Collaborator Author

@tstirrat15 thanks for taking a look :). I'll leave the prop checking as is for now. Might clean it up later on.

@edorivai edorivai merged commit 2e6497a into ReactTraining:next Mar 25, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants