facebook / prop-types

Runtime type checking for React props and similar objects
MIT License
4.48k stars 356 forks source link

Production build each propType is same instance of shim #231

Closed AndyOGo closed 2 years ago

AndyOGo commented 5 years ago

We use this propTypes for custom elements to make runtime parsing based upon prop-types.

Unfortunately in Production the following is true:

import PropTypes from 'prop-types';

PropTypes.string === PropTypes.bool === PropTypes.number === PropTypes.array === PropTypes.object
// -> true

Though this should be false.

Currently the shim function is static, It would be better to generate it or use a shim factory. https://github.com/facebook/prop-types/blob/master/factoryWithThrowingShims.js#L15-L59

ljharb commented 5 years ago

See #106.

AndyOGo commented 5 years ago

Thanks for your immediate feedback.

@gaearon @ljharb @sophiebits I really hope that you give me a chance to alter your decision and viewpoint about this topic.

If we combine your great approach and this PR we can get both benefits of both worlds - better stripped down production code or dev-only code and if someone like me or others have the crazy idea to use awesome prop-types for type casting stuff with custom elememt's attributes or whatever. Plus we have 100% consistent types/pureness between dev and prod. ATM this is not consistent and purity is lost, at dev all PropTypes functions are unique and at prod they are equal.

I would also argue since you decided to make prop-types it's own package, one of it's intention was to see wider utilization, which comes naturally with more use cases and support for other needs.

What do you think?

AndyOGo commented 5 years ago

@sophiebits @gaearon I'm sure you have lots of stuff on your todo-list.

I'm really convinced that you do a great job. Though I'm sure you allowed yourself a little mistake here and I would really insist and appeal to you to correct it. And follow your really nice purity, consistency attitude I know well from other projects of yours.

Hope we can settle this quickly.

AndyOGo commented 5 years ago

Just in case anyone else has this issue, you don't need to fork prop-types.

import PropTypes from 'prop-types';

// guard for Uncaught RangeError: Maximum call stack size exceeded in prod
let level = 0;

// @todo remove as soon as https://github.com/facebook/prop-types/issues/231 is resolved
function getShim(propType) {
  function shim(...args) {
    return propType(...args);
  }

  if (level === 0) {
    level++; // eslint-disable-line no-plusplus
    // make sure to also shim `isRequired`, etc.
    Object.keys(propType)
      .map((key) => {
        shim[key] = propType[key];

        return key;
      })
      .reduce(shimKeys, shim);
    level--; // eslint-disable-line no-plusplus
  }

  return shim;
}

function shimKeys(propTypes, key) {
  const propType = propTypes[key];

  if (typeof propType === 'function') {
    propTypes[key] = getShim(propType);
  }

  return propTypes;
}

const ReactPropTypes = PropTypes;

Object.keys(ReactPropTypes).reduce(shimKeys, ReactPropTypes);

export default ReactPropTypes;
ljharb commented 2 years ago

Closing per https://github.com/facebook/prop-types/pull/106#issuecomment-398443259, but it's fine to continue discussion if there's anything new.