Skip to content
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

PropTypes not always removed from stateless function components #66

Closed
necolas opened this issue Nov 17, 2016 · 10 comments
Closed

PropTypes not always removed from stateless function components #66

necolas opened this issue Nov 17, 2016 · 10 comments
Labels

Comments

@necolas
Copy link

necolas commented Nov 17, 2016

We have a file containing several stateless function components, and one of those components is not having its PropTypes removed. I can't see why, since you have test cases for similar situations. This is what the file roughly looks like:

import CustomPropTypes from 'custom-prop-types';
import React from 'react';

export const Message = ({ className, isSent }) => {
  return (
    <div className={className} />
  );
};

Message.propTypes = {
  className: PropTypes.string,
  isSent: PropTypes.bool
};

/* …4 more components */

const FullMessage = ({ className, entry, isSent }) => {
  return (
    <div className={className} />
  );
};

// these PropTypes are never removed
FullMessage.propTypes = {
  className: PropTypes.string,
  entry: CustomPropTypes.entry.isRequired,
  isSent: PropTypes.bool
};

export default FullMessage;

There are the babel dependencies

"babel-plugin-transform-react-remove-prop-types": "0.2.10"
"babel-plugin-transform-runtime": "6.15.0"
"babel-preset-es2015": "6.18.0"
"babel-preset-react": "6.16.0"
"babel-preset-stage-1": "6.16.0"
"babel-core": "6.13.2" // because in versions >6.13.2 we can't set breakpoints in dev tools
"babel-runtime": "6.11.6"
@oliviertassinari
Copy link
Owner

I have tried reproducing your example with no luck.

npm babel-plugin-transform-runtime@6.15.0 babel-preset-es2015@6.18.0 babel-preset-react@6.16.0 babel-preset-stage-1@6.16.0 babel-core@6.13.2 babel-runtime@6.11.6

How is this source code transpiled?

@necolas
Copy link
Author

necolas commented Nov 17, 2016

How is this source code transpiled?

Via webpack@1.3.2 and babel-loader@6.2.7. The full list of compilation dependencies is quite large.

@oliviertassinari
Copy link
Owner

I'm clueless here 😕 . It could be linked to a conflict with another Babel plugin.
I'm curious to see what the actual transpiled code looks like.
I assume you are not using the wrap mode.

@necolas
Copy link
Author

necolas commented Nov 17, 2016

We're not using wrap. The compiled function in the production bundle is as follows (PropTypes visible at the end).

C=function(e){var t=e.className,n=e.entry,a=e.isSent,r=e.withCaret,o=n.message_data,i=n.error,s=n.message_data.attachment||{},u=(0,l.default)({},o,{entities:o.entities||{},error:i}),d=s.tweet,f=s.photo||s.animated_gif||s.video,p=d?x:f?S:T,m=E(u)?c.default.displayMode.Fill:c.default.displayMode.Stroked,h=a?{direction:"right",displayMode:m,color:i?"red":"textBlue"}:{direction:"left",displayMode:m,color:m===c.default.displayMode.Fill?"fadedGray":"lightGray"},_=!(E(u)&&n.isEmojiOnly),v=b.default.createElement(p,{className:t,entry:n,isSent:a,messageData:u,withMessageBubble:_}),g=f&&!E(u);return r&&!g&&_&&(v=b.default.createElement(c.default,h,v)),v};C.propTypes={className:w.PropTypes.string,entry:v.default.dmEntry.isRequired,isSent:w.PropTypes.bool,withCaret:w.PropTypes.bool};

@oliviertassinari
Copy link
Owner

oliviertassinari commented Nov 17, 2016

Thanks for the compiled code, that was quite useful! Here is a simple reproduction test case:

const Foo11 = () => (
  true && <div />
);

Foo11.propTypes = {
  foo: React.PropTypes.string
};

That's embarrassing.

Oh cool, that's a piece of https://mobile.twitter.com/, good job with it 😄 !

@necolas
Copy link
Author

necolas commented Nov 17, 2016

Nice, thanks for tracking down the issue!

oliviertassinari added a commit that referenced this issue Nov 18, 2016
…lement

We are traversing the return statement deep down.
I still expect some case not behing catched by this approach.
But I would rather not take them into account for now.

Fix #66.
oliviertassinari added a commit that referenced this issue Nov 18, 2016
…lement

We are traversing the return statement deep down.
I still expect some case not being caught by this approach.
But I would rather not take them into account for now.

Closes #66.
oliviertassinari added a commit that referenced this issue Nov 18, 2016
…lement

We are traversing the return statement deep down.
I still expect some case not being caught by this approach.
But I would rather not take them into account for now.

Closes #66.
@mkretschek
Copy link

Hi, I'm also having an issue with getting this working with stateless function components.

Original code:

const { createElement, PropTypes } = require('react');

const el = createElement;

const Foo = ({ bar }) => {
  return el('span', null, bar);
};

Foo.propTypes = {
  bar: PropTypes.string,
};

module.exports = Foo;

Compiled code:

const{createElement,PropTypes}=require('react'),el=createElement,Foo=({bar:a})=>{return el('span',null,a)};Foo.propTypes={bar:PropTypes.string},module.exports=Foo;

Does it require JSX by any chance? I´m using this together with browserify, babelify and the babili preset, if it matters somehow.

If I create a class component, it works just fine, e.g.:

const { createElement, PropTypes, PureComponent } = require('react');

const el = createElement;

class Foo extends PureComponent {
  render() {
    return el('span', null, this.props.bar);
  }
}

Foo.propTypes = {
  bar: PropTypes.string,
};

module.exports = Foo;

Becomes:

const{createElement,PropTypes,PureComponent}=require('react'),el=createElement;class Foo extends PureComponent{render(){return el('span',null,this.props.bar)}}module.exports=Foo;

@oliviertassinari
Copy link
Owner

oliviertassinari commented Mar 27, 2017

@mkretschek Without looking at what's actually going on, my best guess would be that we don't resolve the el = createElement indirection. So we don't consider the Foo function as a React stateless functional component.

@mkretschek
Copy link

mkretschek commented Mar 27, 2017

Thanks for the fast reply. : )

Unfortunately, I'm getting the same result with createElement or react.createElement (without any indirection). Any idea what else could I try or what should I check?

@oliviertassinari
Copy link
Owner

@mkretschek The issue comes from here. We look for the React.createElement pattern. It's a heuristic to determine if the function is a stateless functional component. Your source code has nothing to do with the one generated by babel. I'm wondering if we should be supporting it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants