mergeProps "assumes" Proxy support is available

[chiefcll]

Describe the bug

Given a browser before Proxy is introduced (Chrome < 49)

export function mergeProps<T extends unknown[]>(...sources: T): MergeProps<T> {
  // [breaking && performance]
  //if (sources.length === 1) return sources[0] as any;
  let proxy = false;
  for (let i = 0; i < sources.length; i++) {
    const s = sources[i];
    proxy = proxy || (!!s && $PROXY in (s as object));
    sources[i] =
      typeof s === "function" ? ((proxy = true), createMemo(s as EffectFunction<unknown>)) : s;
  }
  if (proxy) {

If any of the sources has a function, Proxy will be used, even if the system doesn't support Proxy. I think a simple & safe solution would be:

const HAS_PROXY = typeof Proxy !== 'undefined';
export function mergeProps<T extends unknown[]>(...sources: T): MergeProps<T> {
  // [breaking && performance]
  //if (sources.length === 1) return sources[0] as any;
  let proxy = false;
  if (HAS_PROXY) {
    for (let i = 0; i < sources.length; i++) {
      const s = sources[i];
      proxy = proxy || (!!s && $PROXY in (s as object));
      sources[i] =
        typeof s === "function" ? ((proxy = true), createMemo(s as EffectFunction<unknown>)) : s;
    }
  }
  if (proxy) {

Your Example Website or App

https://lightning-tv.github.io/solid-demo-app/#/

Steps to Reproduce the Bug or Issue

This is hard to reproduce without lambdatest account. But open https://lightning-tv.github.io/solid-demo-app/#/ in Chrome < 49

Expected behavior

For proxies not to be used when unavailable.

Screenshots or Videos

Platform

Using Lambdatest with Chrome 42 - As long as the component doesn't have a prop with a function, it works.

Additional context

I'm working on getting SolidJS going on more Chrome 38 browser devices. So I'll be tracking down more things like this. Any support is greatly appreciated 👍

[ryansolid]

Ok for sure.. I guess I'm ok with this position. There are situations where you need proxies to get the updates. It's just not possible without so I thought I had detected those situations adequately so that as long as you did the right things you'd be fine.

A better solution might be just to throw? Like we either error in the cases I've determined proxies are necessary or we silently not handle updates perfectly which this change would suggest.

[chiefcll]

It looks like the proxy is needed if a function is used as one of the sources - not sure entirely what this function does. I don't think I ever "update" a function so I probably don't need the proxy. But in this case even if it is needed, it's not there, so an error is currently thrown. I'd prefer a console.warn with some "Function detected but no proxy so we can't auto update it if you do change it" type message so I don't need to handle the thrown error (also throwing an error because you don't have proxy supports seems wrong...).

[ryansolid]

Ok warning it is then.

It's this case:

<native {...someSignal()} />

In this scenario we have no idea what shape we are going to end up with. With non-functions the object needs to be a proxy for it to change shape, otherwise we can assume that it has exactly the properties it had at creation time.. you can only really update the property values but not change the properties on the object itself. I can think perhaps how with defineProperty people could get around that but like then they can't have any expectation of listening to a property before it exists could ever work (the way it does with a proxy).

[chiefcll]

👍

FYI I'm running into this when I have

<node onLoad={someFunction} />

It sees there is a function so it's setting proxy=true.

[chiefcll]

OK - I see now. I had

export function TileRow(props: TileRowProps) {
  return (
    <Row {...props} style={styles.Row}>
      <Index each={props.items}>{(item) => <Thumbnail {...item} />}</Index>
    </Row>
  );
}

Notice the index which makes item a signal, but I wasn't calling it with {...item()} - Seems Solid magically handles that spread with it being a signal. I think that was also causing the "Make this a Proxy" not the passing in a onLoad={someFunction} - I switched to a loop.

[chiefcll]

And just so you know, Solid still works on Chrome 38

[philippe-wm]

To be more precise, the issue in mergeProps is due to core-js' Symbol polyfill implementation causing $PROXY in s to be always true.

[philippe-wm]

As a side consideration, I also target these old devices, but I override Proxy to fail early: we prefer not discovering Proxy mis-use until we run the app on an old browser.

[chiefcll]

Also can confirm that is the root cause. Solution is still the same. Just have a check if proxy is available on the system.

[ryansolid]

👍

FYI I'm running into this when I have

<node onLoad={someFunction} />

It sees there is a function so it's setting proxy=true.

This is odd. Because there is no spread there. So what we should be getting is a function being pass to the setProperty call.

<node onLoad={someFunction} {...somethingISpread} />

Something like that with a spread is more or less spread({ onLoad: someFunction }, somethingISpread). The issue should be when somethingISpread is a function, not when any individual prop is a function.

But I get the problem. Symbol inclusion checks are always true with the polyfill so it incorrectly identifies the case always. So we should fix that.

[chiefcll]

Sorry for the confusion. I'll double check this case... debugging in a virtual machine with Chrome 38 is not fun.

@philippe-wm statement is accurate and primary case we need solving for and the PR I opened fixes that.

#2284