New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Instead of removing propTypes, replace with empty object #95

Closed
RangerMauve opened this Issue Apr 12, 2017 · 18 comments

Comments

5 participants
@RangerMauve

RangerMauve commented Apr 12, 2017

Removing propTypes entirely is causing issues with third party dependencies that need them to be defined. This was causing issues when using react-native-web.

It was brought up that replacing propTypes with an empty object would have the effects of not needing those definitions, while allowing libraries to reference undefined properties on propTypes and prevent cannot read [name] of undefined errors.

@oliviertassinari

This comment has been minimized.

Owner

oliviertassinari commented Apr 12, 2017

It was brought up that replacing propTypes with an empty object

Won't that potentially introduce sneaky issues? Some library actually use the object to perform logic not just runtime type checking.

What about changing the default property of ignoreFilenames to ['node_modules']? Most people don't parse the node modules so that would only impact few users. Besides, I would rather see lib author using the wrap mode before publishing the package to npm.

@RangerMauve

This comment has been minimized.

RangerMauve commented Apr 12, 2017

lib author using the wrap mode

That's exactly what's happening here with react-native-web. This means that when we're doing development builds, everything relying on it is working fine, but as soon as we tried to do a production build, the libraries relying on propTypes broke.

With regards to the sneaky issues that could arise from people using propTypes for something other than validation, I agree that that would be a problem in certain cases, but I'm not seeing patterns like that showing up as frequently and would want to take the risk since it'll provide more out of the box support.

Would it be okay to have this behavior be opt-in so that projects willing to take the risk can take it and those that don't, don't?

I was looking at the code in an attempt to make a PR for this feature, but I was unsure as to how the ternary expression was being inserted. I saw that wrapperIfTemplate was generating an if statement based on the current NODE_ENV, but I wasn't seeing anything that would generate the ternary.

@virgofx

This comment has been minimized.

virgofx commented Apr 12, 2017

The purpose of the plugin is to remove prop types. That's what it should do. If a 3rd party library is using the plugin but expecting prop types .. then that's their issue.

Using proper babel environments one can define the necessary requirements (dist should use this plugin, but development, normal imports should not). There are numerous examples of vendors using this properly allowing prop types in development, internal builds, but excluding for dist versions. Please see those.

[close issue please]

@necolas

This comment has been minimized.

necolas commented Apr 12, 2017

Won't that potentially introduce sneaky issues? Some library actually use the object to perform logic not just runtime type checking.

RN is starting to export propTypes as their own modules (e.g. ViewPropTypes) which can be used for that use case. But using Component.propTypes anywhere outside of propTypes definition is an anti-pattern - export the propTypes and use that instead. What should be possible is that apps build correctly without using this plugin, even when trying to access Component.propTypes in the propTypes of their own components. Component.propTypes is expected to be an object. If this plugin replaced with {}, then at least accessing like Component.propTypes.style won't error.

@oliviertassinari

This comment has been minimized.

Owner

oliviertassinari commented Apr 15, 2017

I'm not seeing patterns like that showing up as frequently and would want to take the risk since it'll provide more out of the box support.

@RangerMauve react-bootstrap is using that pattern in quite some occasions. Still, I agree that it's not frequent.

RN is starting to export propTypes as their own modules

@necolas Oh, that's great! I have been hitting that wall in the past 💣 . I was trying to go in the opposite direction by throwing a syntax error with the following pattern:

import { SomeComponent } from './SomeComponent';

const propTypes = {
  value: SomeComponent.propTypes.value,
};

But I reverted as

  1. react-native was encouraging people to use the imported component propTypes cc @vjeux
  2. @airbnb pushed for adding a rule at the eslint-plugin-react level that prohibit the use of foreign prop types. (Actually, we could borrow their implementation for the AST traversal 😄 )

But using Component.propTypes anywhere outside of propTypes definition is an anti-pattern - export the propTypes and use that instead.

I think that the whole community would agree on that point 👍 . I'm not sure about the best path going forward. That's not a call I can make on my own.

  • Option 1. We don't change anything, that a bold statement, the community should take into account that accessing Component.propTypes.x can throw an error. People might have to apply fixes like the one accepted by @obipawan.
  • Option 2. We use the safe option, we replace the definition with a {} that can lead to a sneaky issue. However, as the wrap mode is targeting lib author and lib author most likely know their source code, they can anticipate this edge case as long as we document it.
@necolas

This comment has been minimized.

necolas commented Apr 15, 2017

Option 2 please. The alternative is that libraries like RNW have to stop using the plugin and do it manually

necolas referenced this issue in obipawan/react-native-hyperlink Apr 18, 2017

Detect if Text.propTypes actually exists for web (#11)
I'm using [react-native-web](https://github.com/necolas/react-native-web) in conjunction with this library in order to support more platforms with the same codebase.

When building our app for development, everything works properly and this library does exactly what we need it to.

However, react-native-web excludes propTypes when you do a production build in order to reduce bundle size and improve performance.

This leads to [issues](necolas/react-native-web#423) when libraries depend on propTypes being defined.

I propose feature-detecting whether Text.propTypes is actually defined so that people using react-native-web, or any bundlers for native that reduce size agressively, without affecting current users.
@necolas

This comment has been minimized.

necolas commented Apr 20, 2017

@oliviertassinari any thoughts?

@virgofx

This comment has been minimized.

virgofx commented Apr 20, 2017

necolas ... Option 2 is not an option as it changes the sole intention of this transform. You must handle this in YOUR APPLICATION.

@oliviertassinari

This comment has been minimized.

Owner

oliviertassinari commented Apr 20, 2017

@necolas option 2 sounds fair to me. I don't see why @virgofx is so opposed to it. That would only affect the wrap mode, used by lib authors.

@virgofx

This comment has been minimized.

virgofx commented Apr 20, 2017

Libraries using this expect the prop types to be removed completely, hashes are built against this, this would invalidate the whole purpose of the plugin. If you wanted to leave them in then , it needs to be an OPT-IN because option 2 breaks backwards compatibility. Otherwise, necolas can create a fork that does something different.

@necolas

This comment has been minimized.

necolas commented Apr 20, 2017

@oliviertassinari thanks, doing it only for wrap sounds good

@RangerMauve

This comment has been minimized.

RangerMauve commented Apr 20, 2017

@virgofx What do you mean by "hashes are built against this"? Can you show an example of code that relies on propTypes to be removed completely in wrap mode?

@ndbroadbent

This comment has been minimized.

ndbroadbent commented Apr 24, 2017

I also found this issue because I need this for react-native-web. In the meantime, would it be possible to add a wrapCondition option? It could be a string, with a default value of process.env.NODE_ENV !== "production".

Then the the react-native-web project could have a .babelrc similar to:

  "plugins": [
    [ "transform-react-remove-prop-types", {
      "mode": "wrap",
      "wrapCondition": "process.env.NODE_ENV !== \"production\" || process.env.PRESERVE_PROP_TYPES"
    }]
  ]

So most react-native-web users would get a version without props, but if they run into a problem, then can just set process.env.PRESERVE_PROP_TYPES in their webpack config.

Alternatively, we could just submit PRs to all the major react-native-* libraries that are causing this problem. I'm not sure how many there are, or how difficult that would be.

@necolas

This comment has been minimized.

necolas commented Apr 24, 2017

@ndbroadbent this problem will go away entirely if propTypes is always an object, negating the need for widespread changes to packages

@ndbroadbent

This comment has been minimized.

ndbroadbent commented Apr 24, 2017

Here's my attempt at replacing propTypes with an object. I've been testing it with my react-native-web app, and it seems to work perfectly.

I'm still finding my way around the Babel AST, so the code is not very good. But it works well for me.

@oliviertassinari

This comment has been minimized.

Owner

oliviertassinari commented Apr 25, 2017

@ndbroadbent Thanks for the proposed fix! I'm gonna port that into the lib.

@oliviertassinari

This comment has been minimized.

Owner

oliviertassinari commented Apr 25, 2017

I have given a shot at this issue with #101. Let me know if that looks good to you.

@necolas

This comment has been minimized.

necolas commented Apr 26, 2017

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment