[web] withUniwind breaks expo-image via `dataSet` prop clobbering

[bradleyayers]

What happened?

I have an <Image> that I re-render and change the source. The image works properly the first few times I change source, but then it stops updating correctly and it just gets stuck on one image and none of the onLoad* handlers fire correctly.

I've narrowed it down to the way withUniwind works and always passes through a dataSet prop even if it's empty (see

uniwind/packages/uniwind/src/hoc/withUniwind.tsx

Line 140 in 4614d75

dataSet={generateDataSet(props)}

).

Then look at the web implementation of ExpoImage https://github.com/expo/expo/blob/42c88e4f0e92eba794f1e454ccd50aa1f0a72adc/packages/expo-image/src/ExpoImage.web.tsx#L174 and you see that it needs the magic expoimage: true value set, but a couple of lines down it can be clobbered by {...props}.

I'd argue this is also a bug in expo-image, in that it doesn't merge together dataSet, however Uniwind could also avoid this by not passing through dataSet unless it's necessary.

Steps to Reproduce

1. Render an <Image source={…} transition={200}> from expo-image (the transition prop is required to reproduce this).
2. Re-render it with a button or some other state to switch the source prop to another value.

Snack or Repository Link

https://github.com/bradleyayers/uniwind-withuniwind-warning/tree/expo-image-source-dataset

Uniwind version

1.10.0

React Native Version

0.86.0

Platforms

Web

Expo

Yes

Additional information 〰

• I've searched for similar issues in this repository and found none

[bradleyayers]

For now I'm using a patch like this:

const toCamelCase = (str) => str.replace(/-([a-z])/g, (_, letter) => letter.toUpperCase());

function generateDataSetProps(props) {
  let dataSet;
  Object.entries(props).forEach(([key, value]) => {
    if (key.startsWith("data-")) {
      (dataSet ??= {})[toCamelCase(key.slice(5))] = value;
    }
  });
  return dataSet == null ? null : { ...props.dataSet, ...dataSet };
};

// …

const withManualUniwind = (Component, options) => (originalProps) => {
  // …
  return /* @__PURE__ */ jsx(
    Component,
    {
      ...props,
      ...generatedProps,
      ...generateDataSetProps(props)
    }
  );
}

However there are a lot of other components using generateDataSet that would benefit from a similar refactor.

[Brentlok]

Hi, thanks for reporting this, it definitely looks like expo bug and I've opened PR to fix that - expo/expo#48821

But returning undefined instead of an empty object will also land in uniwind

[Brentlok]

PR has been merged in expo as well

[bradleyayers]

I just had a look at #629 and that by itself isn't enough to avoid the clobbering, because

uniwind/packages/uniwind/src/hoc/withUniwind.tsx

Line 88 in 7611bf0

dataSet={generateDataSet(props)}

unconditionally passes the dataSet prop. In my patch I avoided passing the prop entirely by making a new generateDataSetProps function that returns props that can be spread in, so it returns { dataSet: … } | undefined

[Brentlok]

#629 and that by itself isn't enough to avoid the clobbering

Agreed, but the issue is in the expo, and spreading props conditionally could harm performance, additionally we want to avoid any "hacks" in the core for 3rd party dependencies

[bradleyayers]

and spreading props conditionally could harm performance

I'm surprised that conditionally spreading could harm performance, could you elaborate on why that might be? In my mind it's no different than the existing spreading of ...props or ...generatedProps, in that those are dynamically created objects with dynamic keys based on the input props.

additionally we want to avoid any "hacks" in the core for 3rd party dependencies

I agree with this mentality, but I'd argue that passing a prop (dataSet) with undefined is itself a hack, as opposed to not passing the prop at all, and that this is what caused the difference in behaviour of the 3rd party expo code. If no dataSet prop was passed then no unintended difference in behaviour would have occurred. I'd say the defensive programming approach of "being conservative with what you send" to 3rd party components would be a better approach for a library like this.

[Brentlok]

I've meant that adding an additional props spreading could harm performance, because I was referring to all places where dataSet is used not only withUniwind, but your message made me realize that we could just pass dataSet conditionally into generatedProps in withUniwind and that's all, we can keep passing dataSet={undefined} to RN's built in components

[github-actions]

New release v1.11.0 is published: https://github.com/uni-stack/uniwind/releases/tag/v1.11.0