Needles should be recursively resolved about styled-tools HOT 9 OPEN

I thought more about the "deep function values" test case as it may not be needed with styled-components. Assuming the result of ifProp is interpolated into a style block, styled-components should deeply resolve the value itself. However other frameworks such as emotion don't resolve interpolated functions in css blocks. As such I think it's best that styled-tools deeply resolves result values for better compatibility with those frameworks.

from styled-tools.

I think deepProp, deepTheme and deepPalette are enough. People could do withProp(deepProp(...)) for the others.

from styled-tools.

Okay I can create a PR for that.

from styled-tools.

Hi @jtmthf. I'm not sure if I got it.

export const contrastText = (bgNeedle: Needle<any>) =>
  withProp(
    [bgNeedle, p('black'), p('white')] as any,
    (bg: string, black: string, white: string) => {
      // bg ends up being a prop getter function here instead of a string.
      return getLuminance(bg) > 0.179 ? black : white;
    },
  );

Are black and white strings?

If you could create some failing tests, I think it would be easier to understand. Either way, I agree with your title withProp should recursively resolve prop getters. So PR is really welcome. :)

from styled-tools.

@diegohaz already on a PR!

In order to be consistent, I think that any function that resolves a prop-getter should deeply resolve the value. As of now, I'm working on ifProp. Here are just the tests that I've already added that were failing originally.

test("deep function argument", () => {
  expect(ifProp(() => props => props.foo, "yes", "no")()).toBe("no");
  expect(ifProp(() => props => props.foo, "yes", "no")({ foo: false })).toBe(
    "no"
  );
  expect(ifProp(() => props => props.foo, "yes", "no")({ foo: true })).toBe(
    "yes"
  );
});

test("deep function values", () => {
  expect(
    ifProp("foo", () => props => props.foo, () => props => props.bar)({
      bar: "bar"
    })
  ).toBe("bar");
  expect(
    ifProp("foo", () => props => props.foo, () => props => props.bar)({
      foo: "foo"
    })
  ).toBe("foo");
});

test("deep function array argument", () => {
  expect(
    ifProp([() => props => props.foo], "yes", "no")({ bar: false, foo: true })
  ).toBe("yes");
  expect(
    ifProp(["bar", () => props => props.foo], "yes", () => "no")({
      bar: false,
      foo: true
    })
  ).toBe("no");
});

from styled-tools.

Reopening as we need to figure out a better solution (the other one introduced breaking changes #57).

from styled-tools.

I think it makes sense that ifProp(prop: string, ...)does not try try to resolve prop deeply as ifProp is only trying to determine the type of the prop and not to resolve its value. This could be fixed by parseObject and parseString not using the prop function internally anymore. In order to preserve dot property lookup, the dot property functionality of prop can be extracted out.

In the case that someone does want ifProp to deeply resolve the prop, they can still use ifProp(prop('transparent')). For switchProp and and withProp, I do think those should deeply resolve their prop as the intent is to consume the value. The downsides of this being both inconsistency with ifProp and technically being a breaking change even though it's hard to imagine a use case where you would not want the needle of switchProp or withProp deeply resolved.

from styled-tools.

What about a deep version of prop?

withProp(deepProp(props => props => props.foo), x => x);
ifProp(deepProp(props => props => props.foo), true, false);
...

from styled-tools.

That might be more clear. As theme and palette only take keys, should a deepTheme and deepPalette be provided, or should the existing functions internally use deepProp?

Additionally would it be worth providing withDeepProp, switchDeepProp, ifDeepProp, and ifNotDeepProp helpers or does that provide little benefit?

from styled-tools.