deck.gl icon indicating copy to clipboard operation
deck.gl copied to clipboard

Avoid adding props to global function prototype. Doing so breaks the react-three-fiber reconciler

Open marsy-invests opened this issue 1 year ago • 8 comments

Closes #9206

Change List

  • create-props.ts Changed createPropsPrototypeAndTypes to bail out if the parent class of the passed component class is the global function prototype so that it doesn't get polluted.

There may well be a better and cleaner way to do this, but this fixes the bug I'm experiencing.

Note that I was not able to get tests to run locally or on my fork via Github Actions, but the errors appear unrelated to my changes and seem to be occurring on some workflow runs on the main repo.

P.S. deck.gl is an absolutely amazing piece of software!

marsy-invests avatar Oct 10 '24 03:10 marsy-invests

There do seem to be some relevant failing tests, does yarn test browser not work for you locally?

felixpalmer avatar Oct 10 '24 07:10 felixpalmer

yarn test browser does indeed work locally - thanks. I can see there's only one failing test if I roll back my change vs 25 failing tests with my change. I'll look further into them to see if I can understand the issue.

The failing tests I saw on the CI build related to missing image files. If I run yarn test locally, I get

file:///redacted/deck.gl/examples/layer-browser/src/data-samples.js:6
import allPoints from '../data/sf.bike.parking.json' assert { type: 'json' };
                                                     ^^^^^^

SyntaxError: Unexpected identifier 'assert'
    at compileSourceTextModule (node:internal/modules/esm/utils:337:16)
    at ModuleLoader.moduleStrategy (node:internal/modules/esm/translators:166:18)
    at callTranslator (node:internal/modules/esm/loader:437:14)
    at ModuleLoader.moduleProvider (node:internal/modules/esm/loader:443:30)

Node.js v22.9.0
error Command failed with exit code 1.

marsy-invests avatar Oct 10 '24 08:10 marsy-invests

Playing with this a bit further, It's starting to look like deck.gl requires attaching props to the global function prototype in order for the layer tests to pass - something that is incompatible with the way the react-three-fiber reconciler works. I need to spend more time familiarising myself with the deck.gl code and what role props play.

marsy-invests avatar Oct 10 '24 10:10 marsy-invests

@marsy-invests Can you pinpoint in the debugger when the function prototype is polluted? Perhaps add a debugger statement to your if statement and see where it breaks and follow the stack trace?

ibgreen avatar Oct 10 '24 12:10 ibgreen

@ibgreen if I change getPropsPrototype (see https://github.com/visgl/deck.gl/blob/master/modules/core/src/lifecycle/create-props.ts#L82) to throw an error when about to pollute the function prototype as follows:

function getPropsPrototype(componentClass, extensions) {
    // A string that uniquely identifies the extensions involved
    let cacheKey = MergedDefaultPropsCacheKey;
    if (extensions) {
        for (const extension of extensions) {
            const ExtensionClass = extension.constructor;
            if (ExtensionClass) {
                cacheKey += `:${ExtensionClass.extensionName || ExtensionClass.name}`;
            }
        }
    }
    const defaultProps = getOwnProperty(componentClass, cacheKey);
    if (!defaultProps) {
        if (componentClass === Function.prototype) {
            throw Error("About to pollute function prototype", componentClass)
        }
        return (componentClass[cacheKey] = createPropsPrototypeAndTypes(componentClass, extensions || []));
    }
    return defaultProps;
}

I get the following stack trace

ErrorBoundary.tsx:29 Uncaught error: Error: About to pollute function prototype
    at getPropsPrototype (chunk-WVBUUDCH.js?v=94ea3cd1:34413:13)
    at createPropsPrototypeAndTypes (chunk-WVBUUDCH.js?v=94ea3cd1:34425:30)
    at getPropsPrototype (chunk-WVBUUDCH.js?v=94ea3cd1:34415:39)
    at createPropsPrototypeAndTypes (chunk-WVBUUDCH.js?v=94ea3cd1:34425:30)
    at getPropsPrototype (chunk-WVBUUDCH.js?v=94ea3cd1:34415:39)
    at createPropsPrototypeAndTypes (chunk-WVBUUDCH.js?v=94ea3cd1:34425:30)
    at getPropsPrototype (chunk-WVBUUDCH.js?v=94ea3cd1:34415:39)
    at createProps (chunk-WVBUUDCH.js?v=94ea3cd1:34385:26)
    at new Component (chunk-WVBUUDCH.js?v=94ea3cd1:34543:18)
    at new Layer (chunk-WVBUUDCH.js?v=94ea3cd1:34918:5) {componentStack: '\n    at Image (http://localhost:5173/src/ui/le…(http://localhost:5173/src/ErrorBoundary.tsx:6:1)'} 

marsy-invests avatar Oct 10 '24 13:10 marsy-invests

And if I add console.log("Calling getPropsPrototype for", componentClass) to the start of getPropsPrototype, here's the logs that are created as it walks the prototype chain as the layer gets created.

Calling getPropsPrototype for class extends CompositeLayer {
  static {
    this.layerName = "GeoJsonLayer";
  }
  static {
    this.defaultProps = defaultProps15;
  }
  initializeState() {
    this.state = {
      layerProps: {},
    …
Calling getPropsPrototype for class extends Layer {
  static {
    this.layerName = "CompositeLayer";
  }
  /** `true` if this layer renders other layers */
  get isComposite() {
    return true;
  }
  /** Returns true if all async res…
Calling getPropsPrototype for class extends Component {
  constructor() {
    super(...arguments);
    this.internalState = null;
    this.lifecycle = LIFECYCLE.NO_STATE;
    this.parent = null;
  }
  static {
    this.defaultProps = d…
Calling getPropsPrototype for class {
  static {
    this.componentName = "Component";
  }
  static {
    this.defaultProps = {};
  }
  constructor(...propObjects) {
    this.props = createProps(this, propObjects);
    this.id = this.p…
Calling getPropsPrototype for ƒ () { [native code] }
chunk-NUM45GRF.js?v=586f8a51:16638 Uncaught Error: About to pollute function prototype
    at getPropsPrototype (chunk-SL5OX4HQ.js?v=586f8a51:34414:13)
    at createPropsPrototypeAndTypes (chunk-SL5OX4HQ.js?v=586f8a51:34426:30)
    at getPropsPrototype (chunk-SL5OX4HQ.js?v=586f8a51:34416:39)
    at createPropsPrototypeAndTypes (chunk-SL5OX4HQ.js?v=586f8a51:34426:30)
    at getPropsPrototype (chunk-SL5OX4HQ.js?v=586f8a51:34416:39)
    at createPropsPrototypeAndTypes (chunk-SL5OX4HQ.js?v=586f8a51:34426:30)
    at getPropsPrototype (chunk-SL5OX4HQ.js?v=586f8a51:34416:39)
    at createPropsPrototypeAndTypes (chunk-SL5OX4HQ.js?v=586f8a51:34426:30)
    at getPropsPrototype (chunk-SL5OX4HQ.js?v=586f8a51:34416:39)
    at createProps (chunk-SL5OX4HQ.js?v=586f8a51:34385:26)

marsy-invests avatar Oct 10 '24 13:10 marsy-invests

Revised the fix. It now passes the test suite. getPropsPrototype now just returns an empty object if the componentClass passed is not actually a Component.

marsy-invests avatar Oct 11 '24 08:10 marsy-invests

Coverage Status

coverage: 91.635% (-0.003%) from 91.638% when pulling 702d715383c8ac2f5a006db4b3df91df14846cc7 on marsy-invests:master into c7f1a66e47062eee85ce30cfe1216d6fb8287aae on visgl:master.

coveralls avatar Oct 11 '24 09:10 coveralls