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

Switch to render prop function pattern #29

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

ragalie
Copy link

@ragalie ragalie commented Jan 13, 2019

Hello! I'd like to use this library with TypeScript (see also #26), but I was having trouble writing definitions with the current implementation.

For example, we want to make sure that PositionLabel receives the cursor props. We can enforce that by specifying that the component expects those properties:

interface DetectedEnvironment {
  isMouseDetected: boolean;
  isTouchDetected: boolean;
}

interface ElementDimensions {
  width: number;
  height: number;
}

interface Position {
  x: number;
  y: number;
}

interface Props {
  detectedEnvironment: DetectedEnvironment;
  elementDimensions: ElementDimensions;
  isActive: boolean;
  isPositionOutside: boolean;
  position: Position;
}

class PositionLabel extends React.Component<Props, State> {
  ...
}

To make this work under the current implementation, though, we need to pass in a "zero-value" template when we instantiate the component. And not just for the component that we want to pass the cursor data into, but any component nested under the ReactCursorPosition component.

<ReactCursorPosition>
  <PositionLabel detectedEnvironment={{isMouseDetected: false, isTouchDetected: false}} elementDimensions=... />
  <InstructionLabel detectedEnvironment={{isMouseDetected: false, isTouchDetected: false}} elementDimensions=... />
</ReactCursorPosition>

Instead of trying to make TypeScript definitions work under the current implementation, I'm opening this PR recommending that we switch to using the render prop function pattern in this library. This is a pattern widely used within the React community, and a particularly good fit for libraries like this one that expose data to other components. In fact, the React documentation on the render prop pattern uses a mouse tracking component as its prototypical example: https://reactjs.org/docs/render-props.html

In this implementation, the children prop to ReactCursorPosition is required to be a function that accepts as arguments the cursor props. The properties can then be passed into the relevant downstream nodes.

I suggest you look at the commits independently, as that's the easiest way to get a handle on what's proposed. These are the two that I'd look at first:

1c4b564: Implements the render prop function pattern and updates the README
3b0be56: Implements type definitions

Thanks for taking a look! I look forward to your feedback.

The function receives as arguments the cursor props, which can then be
passed into the relevant downstream nodes.

This approach makes it straightforward to implement TypeScript
definitions and is in line with the approach taken by many react-based
libraries.

In fact, the react documentation on render props uses a mouse tracking
component as its prototypical example: https://reactjs.org/docs/render-props.html
@coveralls
Copy link

coveralls commented Jan 13, 2019

Coverage Status

Coverage remained the same at 100.0% when pulling 642c719 on ragalie:render-prop into 0f594bb on ethanselzer:master.

@ethanselzer
Copy link
Owner

@ragalie - This looks amazing! What are your thoughts around retaining the previous "component decorator" pattern for backward compatibility? Consumers would have the choice of using either pattern.

@ragalie
Copy link
Author

ragalie commented Feb 9, 2019

If you think the render prop pattern is a better long-term fit (I think it is!), then I'd recommend replacing the component decorator pattern with the render prop one and not making both available going forward.

If we keep both around, then we either develop for both paths (which adds time and complexity to improvements), or we only develop for the render prop path, in which case the functionality is the same between 3.0.3 and 4+ along the component decorator path.

So unless we're planning on developing for both paths in the future (which I wouldn't recommend), it simplifies things for maintainers and for consumers to maintain 3.0.x using the component decorator pattern and to release 4+ using the render prop pattern.

@dguay
Copy link

dguay commented Apr 2, 2020

Is there still no typescript definition for this lib?

Unnecessary now that we're outputing to a function; just don't do
anything with the arguments if you don't want to.
The caller can transform the props themselves via the function that
receives them.
@ragalie
Copy link
Author

ragalie commented May 18, 2020

I'd still love to see this get merged in. @ethanselzer what do you think?

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

Successfully merging this pull request may close these issues.

4 participants