Needles should be recursively resolved

[jtmthf]

I ended up finding a solution to my problem while writing this issue, but I believe it's something that styled-tools should handle by default. I'm working with the sample below which a reakit theme, and the issue is in contrastText. What ends up happening is that bg resolves to a prop-getter function instead of a string. The reason being that bgNeedle is a prop-getter that resolves to a prop-getter which is not supported by withProp. What I ended up doing is writing a custom implementation of withProp (below the example) that recursively resolves prop-getter functions. It would be useful if this was either the default behavior of withProp or was exposed as a separate function.

// utils.ts

import { range } from 'lodash-es';
import { getLuminance, lighten } from 'polished';
import { Needle, palette as p, withProp } from 'styled-tools';

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;
    },
  );

export const contrastPalette = (palette: string, tone?: number) =>
  contrastText(p(palette, tone));

export const lightenPalette = (
  amount: number,
  palette: string,
  tone?: number,
) =>
  withProp([p(palette, tone)] as any, (color: string) =>
    lighten(amount, color),
  );

export const tonePalette = (palette: string) => [
  p(palette),
  ...range(0.1, 0.5, 0.1).map(i => lightenPalette(i, palette)),
];

export const tonePaletteText = (palette: string) =>
  range(5).map(i => contrastPalette(palette, i));

export const tonePaletteWithText = (name: string, palette: string) => ({
  [name]: tonePalette(palette),
  [`${name}Text`]: tonePaletteText(name)
});

// index.ts

import { tonePaletteWithText } from './utils';

export const palette = {
  white: '#fff',
  black: '#000',

  astronautBlue: '#003a5d',

  ...tonePaletteWithText('primary', 'astronautBlue'),
};

Solution

// simple deepWithProp

const deepWithProp = <Props extends object, T = undefined>(
  needle: Needle<Props> | Needle<Props>[],
  fn: (...args: any[]) => T
) => (props = {}): T => {
  if (Array.isArray(needle)) {
    const needles = needle.map(arg => deepWithProp(arg, x => x)(props));
    return fn(...needles);
  }
  if (typeof needle === "function") {
    return fn(deepWithProp(needle(props as Props), x => x)(props));
  }
  return fn(prop(needle, needle)(props));
};

[diegohaz]

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. :)

[jtmthf]

@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");
});

[jtmthf]

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.

[diegohaz]

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

[jtmthf]

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.

[diegohaz]

What about a deep version of prop?

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

[jtmthf]

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?

[diegohaz]

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

[jtmthf]

Okay I can create a PR for that.