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

propTypes & defaultProps removed from decorated components #156

Open
thany opened this issue Jun 11, 2020 · 5 comments
Open

propTypes & defaultProps removed from decorated components #156

thany opened this issue Jun 11, 2020 · 5 comments
Labels

Comments

@thany
Copy link

thany commented Jun 11, 2020

Say I've got this component:

const Awesome = props => <div>awesomeness</div>;

Awesome.propTypes = {
  title: PropTypes.string
};

It really doesn't matter.

I export this component wrapped in the track decorator:

export default track(() => ({
  eventCategory: 'Awesome',
  eventAction: 'Awesome action',
}))(Awesome);

Then, in a different component, let's call it Tremendous, I'm trying to inherit propTypes from Awesome into Tremendous.

const Tremendous = ({awesomeProps}) => <Awesome {...awesomeProps} />;

Tremendous.propTypes = {
  awesomeProps: PropTypes.shape(Awesome.propTypes)
};

This won't work. In the above example, Awesome.propTypes is undefined. The reason for this is the track decorator. Had I not used it, I could inherit the propTypes like above, just fine.

TL;DR: The track decorator should copy/preserve propTypes and defaultProps.

Versions:
React 16.13.1
React-tracking 7.1.0
Node.js 12.18

@tizmagik
Copy link
Collaborator

tizmagik commented Jul 8, 2020

Hey, thanks for creating this issue and clearly describing what you're seeing! Apologies for my late response!

I think this is actually expected behavior. We're hoisting non-React statics so that any static properties on the Component bubble up through the decorator, but for React-specific static properties the common pattern is to keep them defined/managed by React (propTypes is one such example) since track is technically a new component in the hierarchy. This is a pattern commonly used for many higher-order components in the wild (e.g. Redux's @connect()).

I think to achieve what you're looking to do, it may be better to explicitly define the PropTypes as an importable dependency, since that's essentially what's going on when you reference Awesome.propTypes from another Component (you're importing "Awesome" as a general namespace but not the individual dependencies). You should be able to achieve what you're looking to do like so:

/* Awesome.js */

const Awesome = props => <div>awesomeness</div>;

// explicitly export it for use outside this component
export const propTypes = {
  title: PropTypes.string
};

// attach those propTypes
Awesome.propTypes = propTypes;

// normal default export of the decorated component
export default track(() => ({
  eventCategory: 'Awesome',
  eventAction: 'Awesome action',
}))(Awesome);

Then in your other component you would use it like so:

/* Tremendous.js */

import Awesome, { propTypes as awesomePropTypes } from "./Awesome"; 
// Note: if you wanted to avoid the rename, you could export it as "awesomePropTypes" in Awesome.js

const Tremendous = ({awesomeProps}) => <Awesome {...awesomeProps} />;

Tremendous.propTypes = {
  awesomeProps: PropTypes.shape(awesomePropTypes) // can now reference awesomePropTypes explicitly
};

@thany
Copy link
Author

thany commented Jul 10, 2020

That's actually what I ended up doing. It's too bad it has to be that way though, because it's a bit more verbose than I'd like it to be.

But if what you're saying is really true, that it's normal to (in my own words) not preserve propTypes, then I'm okay with that. I just really thought it was a bug.

@tizmagik
Copy link
Collaborator

It's not that it doesn't preserve propTypes, it does, since we actually pass them through (the props you expect to be able to set on a tracking decorated component are still there from React's perspective), but it's not an exported member on the module. That's why it's undefined when you try to access that property name, "propTypes" directly, it's better to explicitly export and import when needed.

It's a good question though and something I'd like to capture in our FAQ, #86

@thany
Copy link
Author

thany commented Jul 15, 2020

The line of code you referred to, just passes the props, not the propTypes. But no worries, mate, I understand what you're trying to say. I agree a FAQ would probably be a good idea.

(finally a FAQ that is true to its meaning 😎)

@tizmagik
Copy link
Collaborator

Yes, you're right, technically it's passing along the props but the propTypes also come along for the ride, but as an implementation detail of how React handles those and not the static propType property on the component itself, true! 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants