nytimes / react-tracking

🎯 Declarative tracking for React apps.
https://open.nytimes.com/introducing-react-tracking-declarative-tracking-for-react-apps-2c76706bb79a
Other
1.88k stars 123 forks source link

propTypes & defaultProps removed from decorated components #156

Open thany opened 4 years ago

thany commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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! 😄