Comments (18)
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]
from babel-plugin-transform-react-remove-prop-types.
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.
from babel-plugin-transform-react-remove-prop-types.
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.
from babel-plugin-transform-react-remove-prop-types.
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.
from babel-plugin-transform-react-remove-prop-types.
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.
from babel-plugin-transform-react-remove-prop-types.
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
- react-native was encouraging people to use the imported component propTypes cc @vjeux
- @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 thewrap
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.
from babel-plugin-transform-react-remove-prop-types.
Option 2 please. The alternative is that libraries like RNW have to stop using the plugin and do it manually
from babel-plugin-transform-react-remove-prop-types.
@oliviertassinari any thoughts?
from babel-plugin-transform-react-remove-prop-types.
necolas ... Option 2 is not an option as it changes the sole intention of this transform. You must handle this in YOUR APPLICATION.
from babel-plugin-transform-react-remove-prop-types.
@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.
from babel-plugin-transform-react-remove-prop-types.
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.
from babel-plugin-transform-react-remove-prop-types.
@oliviertassinari thanks, doing it only for wrap sounds good
from babel-plugin-transform-react-remove-prop-types.
@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?
from babel-plugin-transform-react-remove-prop-types.
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.
from babel-plugin-transform-react-remove-prop-types.
@ndbroadbent this problem will go away entirely if propTypes is always an object, negating the need for widespread changes to packages
from babel-plugin-transform-react-remove-prop-types.
@ndbroadbent Thanks for the proposed fix! I'm gonna port that into the lib.
from babel-plugin-transform-react-remove-prop-types.
I have given a shot at this issue with #101. Let me know if that looks good to you.
from babel-plugin-transform-react-remove-prop-types.
Thanks!
from babel-plugin-transform-react-remove-prop-types.
Related Issues (20)
- 0.4.16 bug: Cannot read property 'referencePaths' of undefined at VariableDeclarator, lib/index.js:292:56 HOT 6
- Cannot read property 'type' of undefined at memberExpressionRootIdentifier HOT 1
- react-portal remove prop-types HOT 2
- mode wrap broken in 0.4.20(#168) HOT 2
- Ability to remove `contextTypes` HOT 6
- "unsafe-wrap" mode crashes when a type is referenced from a variable
- Support createReactClass alternatives via config HOT 4
- Cannot get "name" of undefined
- Is it supports react-native? HOT 9
- Destructured PropTypes are not removed HOT 4
- Doesn't work with react-navigation library
- Issue when used with `createReactClass` HOT 1
- Option to include only certain files
- Support for configure wrap condition HOT 4
- Rename wrongly in switch/case statement
- Not working for SFC forwardRef() and createContext()
- Doesnβt remove propTypes in certain situations HOT 2
- Transform error with mode=wrap for TypeScript class component static propTypes type definition HOT 4
- How to use it with multiple environments in babel-env?
- swc support HOT 1
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
π Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. πππ
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google β€οΈ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from babel-plugin-transform-react-remove-prop-types.