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

shouldForwardProp support #3006

Merged

Conversation

stevesims
Copy link
Contributor

@stevesims stevesims commented Feb 7, 2020

Following discussion elsewhere, such as on #2780, re-opening this PR (previously #2392)

adds support for use of shouldForwardProp config method, which can be used to explicitly determine which props should be forwarded on
if this config is not provided then existing functionality is preserved (i.e. all props passed to components, or for HTML elements check using validAttr)

Inspired by the same feature in emotion

Resolve #439
Resolve #2878

@stevesims
Copy link
Contributor Author

stevesims commented Feb 7, 2020

Code now re-worked a bit and rebased on current master

@stevesims stevesims force-pushed the feat/should-forward-prop branch from 4072f9d to 1cbf75a Compare Feb 7, 2020
@stevesims stevesims requested a review from kitten Feb 7, 2020
@penx

This comment has been minimized.

@diondiondion
Copy link

diondiondion commented Feb 7, 2020

Does this support statically forwarding the as prop, too?

In other words, will a component as defined below pass the as prop on to the wrapped styled component without having to use the forwardedAs prop instead?

const Comp = styled(AnotherStyledComponent).withConfig({
  shouldForwardProp: prop => ['as'].includes(prop)
})`
  /* ... */
`;

@penx
Copy link

penx commented Feb 7, 2020

@diondiondion what would a unit test look like for that? Something like this?

    it('supports statically forwarding the as prop', () => {
       const StyledDiv = styled('div')`
         color: red;
       `;
       const StyledDiv2 = styled('div')`
         color: blue;
       `;

       const ComposedDiv = styled(StyledDiv).withConfig({
         shouldForwardProp: prop => !['as'].includes(prop)
       })`
  /* ... */
`;
       const wrapper = TestRenderer.create(<ComposedDiv as={StyledDiv2} />);
       const { props } = wrapper.root.findByType('div');
       // expect ?
     });

@stevesims
Copy link
Contributor Author

stevesims commented Feb 7, 2020

@diondiondion good question - and to be honest, I'm not sure. I did wrote some tests concerning how as works, but I'm not entirely sure they cover what you're asking about. Unfortunately I don't have the time to check on this right now, and won't until Monday at the earliest.

@probablyup
Copy link
Contributor

probablyup commented Feb 7, 2020

@diondiondion
Copy link

diondiondion commented Feb 8, 2020

I realise now that the example in my previous question had a typo in it, it was set up to prevent the as prop from being passed on. I've edited it now.

what would a unit test look like for that? Something like this?

@penx Let's use an intermediate JSX component as an example as that's the most likely real world use case I can think of.

it('supports statically forwarding the as prop', () => {
	const Clickable = styled('a')`
		background-color: pink;
	`;
	const Button = ({as, className, children}) => (
		<Clickable as={as} className={className}>Hello {children}</Clickable>
	);

	const MenuButton = styled(Button).withConfig({
		shouldForwardProp: prop => prop === 'as'
	})`
		margin-right: 4px;
	`;

	const wrapper = TestRenderer.create(<MenuButton as="button">world</MenuButton>);
});

Given this example, I'd expect The rendered component's

  • element type to be "button"
  • text content to equal "Hello world"
  • styles to be background-color: pink; margin-right: 4px;

Fwiw we would not deprecate forwardedAs so might not be an issue

@probablyup In my mind forwardedAs has one valid use case – when it's used at the same time as the as prop.

However, the scenario where you have to use it instead of the as prop because not doing so would break the component or its styles is why I'm advocating for it. In those cases, forwarding the as prop statically would be a MUCH safer option. The lack of this ability creates inconsistent component APIs that unnecessarily require knowing the internals of a component.

@probablyup
Copy link
Contributor

probablyup commented Feb 8, 2020

@diondiondion
Copy link

diondiondion commented Feb 8, 2020

Not sure how this is a "subtractive API change", but alright. But wouldn't not introducing this capability from the get go make adding it later a potential breaking change? It seems like that's how it should work. shouldForwardProp: prop => prop === 'as' reads pretty obvious to me.

@karlhorky

This comment has been minimized.

@@ -180,6 +192,20 @@ function useStyledComponentImpl<Config: {}, Instance>(
return createElement(elementToBeCreated, propsForElement);
}

function getShouldForwardProp(options, target, isTargetStyledComp) {
Copy link
Member

@kitten kitten Feb 13, 2020

Choose a reason for hiding this comment

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

nit: Let's maybe keep this inlined next to finalAttrs for the sake of having them all in one place. I'd argue we can clean this up in a separate refactor PR and get rid of some of the legacy stuff in this file.

Copy link
Contributor Author

@stevesims stevesims Feb 14, 2020

Choose a reason for hiding this comment

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

👍

@@ -75,6 +75,7 @@ interface StyledComponentWrapperProperties {
displayName: string;
foldedComponentIds: Array<string>;
target: Target;
shouldForwardProp: Function;
Copy link
Member

@kitten kitten Feb 13, 2020

Choose a reason for hiding this comment

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

We can introduce a type for this, since it's just (prop: string) => boolean let's also remember that this is optional and may be undefined

Copy link
Contributor Author

@stevesims stevesims Feb 14, 2020

Choose a reason for hiding this comment

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

👍

// i.e. Don't pass through non HTML tags through to HTML elements
// else (we're on a component - pass through all by default)
// () => true
const shouldReallyForwardProp = shouldForwardProp || (
Copy link
Member

@kitten kitten Feb 13, 2020

Choose a reason for hiding this comment

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

I think the comment here is redundant if this was a little clearer in the first place 😅

If we do a little refactor we can clean this up and make what's going on a lot clearer.

  • shouldFilterProps could be reverted and we can remove isTargetTag from it.
  • then wee can introduce a separate variable: const propFilterFn = shouldForwardProp || (isTargetTag && validAttr);.
  • if (shouldFilterProps) becomes if (shouldFilterProps || propFilterFn)
  • Then shouldReallyForwardProp(key) (which btw has unnecessary parens) can be replaced by: (!propFilterFn || propFilterFn(key))
  • lastly we can now rename shouldFilterProps to shouldFilterComputedProps

Copy link
Contributor Author

@stevesims stevesims Feb 14, 2020

Choose a reason for hiding this comment

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

done - was a little bit more involved, but essentially followed your suggestions - also didn't rename shouldFilterProps cos that didn't feel entirely worth it 😁

return target.shouldForwardProp;
}

return options.shouldForwardProp;
Copy link
Member

@kitten kitten Feb 13, 2020

Choose a reason for hiding this comment

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

@probablyup This can probably be done separately but we can totally add shouldForwardProp as a shortcut to replace the manual withConfig, right? Just to have alignment with Emotion.

I'd also argue now that we're touching filtering props, "transient props" (props that are always filtered) could also be revived after merging this. (Ref: #2093)

Steve Sims and others added 2 commits Feb 14, 2020
adds support for use of `shouldForwardProp` config method, which can be used to explicitly determine which props should be forwarded on
if this config is not provided then existing functionality is preserved (i.e. all props passed to components, or for HTML elements check using `validAttr`)

Inspired by the same feature in emotion
following feedback from @kitten, `shouldForwardProp` now has a proper flow-type definition
refactor how `shouldForwardProp` is used to make code slightly clearer
code to compose `shouldForwardProp` now in-lined into `createStyledCompontent`
@stevesims stevesims force-pushed the feat/should-forward-prop branch from 1cbf75a to 94b2d76 Compare Feb 14, 2020
// $FlowFixMe
shouldForwardProp = prop => target.shouldForwardProp(prop) && options.shouldForwardProp(prop);
}
({ shouldForwardProp } = target);
Copy link
Member

@kitten kitten Feb 14, 2020

Choose a reason for hiding this comment

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

This doesn’t seem right maybe, 🤔 was this supposed to be in an else statement?

nit: Also can we drop the destructuring here please? We’re also not destructuring for the arrow function composition above, so let’s maybe settle on one pattern for both?

Copy link
Contributor Author

@stevesims stevesims Feb 14, 2020

Choose a reason for hiding this comment

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

heh - i didn't want to use destructuring here - eslint complained at me 😉 happy to drop

Copy link
Contributor Author

@stevesims stevesims Feb 14, 2020

Choose a reason for hiding this comment

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

and yes - should have had an else in there - good spot 😁

kitten
kitten approved these changes Feb 14, 2020
Copy link
Member

@kitten kitten left a comment

This is looking amazing! 👏 thanks for the time to submit this and to get it updated ♥️

Once we’ve got this merged I hope we can internally discuss and move on to add a shortcut replacing the manual usage of withConfig. I hope to be able to merge this later today 🍀

@ajkl2533
Copy link

ajkl2533 commented Feb 18, 2020

Hi there do you have any timeline when this will be released?

@stevesims
Copy link
Contributor Author

stevesims commented Feb 18, 2020

@diondiondion FYI i've not been ignoring your question, and have looked into it quite deeply.

There's an issue with the test that you've proposed:

it('supports statically forwarding the as prop', () => {
	const Clickable = styled('a')`
		background-color: pink;
	`;
	const Button = ({as, className, children}) => (
		<Clickable as={as} className={className}>Hello {children}</Clickable>
	);

	const MenuButton = styled(Button).withConfig({
		shouldForwardProp: prop => prop === 'as'
	})`
		margin-right: 4px;
	`;

	const wrapper = TestRenderer.create(<MenuButton as="button">world</MenuButton>);
});

Given this example, I'd expect The rendered component's

  • element type to be "button"
  • text content to equal "Hello world"
  • styles to be background-color: pink; margin-right: 4px;

The problem is that by providing an as prop to MenuButton you're telling that component to use a different component to render than the specified Button component. As a result your text content will be just world, with only the margin-right style applied.

Without using forwardedAs, the only way for the system to know that it should ignore the as prop being passed to MenuButton would be if the passed in shouldForwardProp function is analysed to see what it does when presented with an as prop. This doesn't feel entirely wise to me.

It seems to me that the current forwardedAs approach is the best compromise to this issue.

@diondiondion
Copy link

diondiondion commented Feb 18, 2020

FYI i've not been ignoring your question, and have looked into it quite deeply.

@stevesims No worries, thanks for looking into it.

The problem is that by providing an as prop to MenuButton you're telling that component to use a different component to render than the specified Button component.

Yes, but that's exactly the point of my request. I want to be able to tell styled-components that MenuButton should never "act" on the as prop and instead directly forward it to the Button component that it wraps (see #2129 for more detail).

The shouldForwardProp option sounds to me like it should be able to handle this, but maybe it's not the right tool for the job. (In which case we'd need an additional option, something like ignoreAs or allowAs like in astroturf; but that'd be a discussion for another place).

Without using forwardedAs, the only way for the system to know that it should ignore the as prop being passed to MenuButton would be if the passed in shouldForwardProp function is analysed to see what it does when presented with an as prop. This doesn't feel entirely wise to me.

Isn't every prop passed to the shouldForwardProp function? I thought the complexity came from making sure that when the as prop is not handled in shouldForwardProp, the current behaviour stays the same, but it didn't seem like an unsurmountable problem.

@Steveeeie
Copy link

Steveeeie commented Feb 21, 2020

Any updates on when this will be released?

@Steveeeie
Copy link

Steveeeie commented Feb 24, 2020

@Martinnord
Copy link

Martinnord commented Feb 27, 2020

I feel with @Steveeeie. Any plans on this being released soon?

@lucastobrazil
Copy link

lucastobrazil commented Feb 29, 2020

Ppppplllleeease can we merge this? i was about to start migrating to Emotion for this very reason but already started missing the feel of s-c.

@aulneau
Copy link

aulneau commented Mar 4, 2020

I've been hoping for this feature to be implemented for so long, would absolutely love to see it merged! Thanks for all the work on this.

@lucastobrazil
Copy link

lucastobrazil commented Mar 4, 2020

@kitten sorry for ping but we have a lot of excited people here. Anything we could do to help get it merged? Docs need to be written or anything?

@probablyup
Copy link
Contributor

probablyup commented Mar 5, 2020

@lucastobrazil Yes a PR for the website to add this API documentation would be great. I think this is ready to be merged though.

Someone will also need to work on the typing implementations for TS and Flow.

@probablyup probablyup merged commit e02109e into styled-components:master Mar 5, 2020
1 check passed
@probablyup
Copy link
Contributor

probablyup commented Mar 5, 2020

We also need a PR for StyledNativeComponent with the same changes.

@peduarte
Copy link

peduarte commented Mar 5, 2020

Ohhhh yes! So so so happy about this. Thank you everyone for your work <3

@lucastobrazil
Copy link

lucastobrazil commented Mar 5, 2020

Just trying this out locally - based on the tests written, i've tried doing this:

const Box = styled.div.withConfig({
  shouldForwardProp: prop => !['font-family'].includes(prop)
})(
  { boxSizing: 'border-box' },
  compose(
    space,
    // ...
  )
);

But it doesn't seem to call the withConfig method?

@Martinnord
Copy link

Martinnord commented Mar 6, 2020

I'm facing the same issue as @lucastobrazil. Is it something we are missing?

@lucastobrazil
Copy link

lucastobrazil commented Mar 8, 2020

@Martinnord I managed to get it working with the local styled components sandbox app that is bundled with the mono repo. Following the example in the test it worked.

Was trying to use yarn link to link my project locally to this package, but it was failing.

Do we have any visibility into the release plan for the next official npm release with this feature in it?

@Steveeeie
Copy link

Steveeeie commented Mar 16, 2020

@probablyup Are you able to give an update as to when this will be part of a release?

@probablyup
Copy link
Contributor

probablyup commented Mar 16, 2020

@tuck2002
Copy link

tuck2002 commented Mar 30, 2020

Near future meaning? Rebuilding a component library with Styled System and this is my biggest hang up at the moment.

@Steveeeie
Copy link

Steveeeie commented Apr 1, 2020

@probablyup can we get an update on when this will be released?

@Martinnord
Copy link

Martinnord commented Apr 6, 2020

image

@aulneau
Copy link

aulneau commented Apr 6, 2020

@probablyup anything we can do to help? seems like a lot of folks are waiting for this and would probs be happy to assist :)

@probablyup
Copy link
Contributor

probablyup commented Apr 6, 2020

I was waiting on #3061 but I think I'll just cut a release later tonight since it's taking a while.

@StevenLangbroek
Copy link

StevenLangbroek commented Apr 6, 2020

Yeah sorry I wouldn't wait on that, need to figure out what the hell is going on before I can submit a fix, and since it is a fix we can cut a patch when it's ready.

@peduarte
Copy link

peduarte commented Apr 6, 2020

Great team effort everyone 👏

@probablyup
Copy link
Contributor

probablyup commented Apr 7, 2020

Out in 5.1, thanks everyone for pitching in! https://github.com/styled-components/styled-components/releases/tag/v5.1.0

@styled-components styled-components locked as resolved and limited conversation to collaborators Apr 7, 2020
@kachkaev
Copy link
Member

kachkaev commented Apr 7, 2020

Discussion of typings for this new feature: DefinitelyTyped/DefinitelyTyped#43693

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

Successfully merging this pull request may close these issues.