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

Add a failing test for sort-prop-types #567

Closed
wants to merge 1 commit into from
Closed

Add a failing test for sort-prop-types #567

wants to merge 1 commit into from

Conversation

oliviertassinari
Copy link
Contributor

E.g, we are using this pattern on the Material-UI repository.

@ljharb
Copy link
Member

ljharb commented Apr 24, 2016

In your example, the propTypes aren't referenced anywhere in the function. Is that reflective of your actual code?

@yannickcr
Copy link
Member

We do not follow variables references, so with this pattern the propTypes order is not checked.

@oliviertassinari
Copy link
Contributor Author

oliviertassinari commented Apr 27, 2016

We do not follow variables references

@yannickcr Will this be fixed in the future? Or should we just not use this pattern?

@oliviertassinari
Copy link
Contributor Author

In your example, the propTypes aren't referenced anywhere in the function. Is that reflective of your actual code?

@ljharb This is the actual code. I have removed the reference. I wanted to be consistent with the other tests.

@yannickcr
Copy link
Member

Will this be fixed in the future? Or should we just not use this pattern?

We only have access to the AST and it is tricky to follow variables references, so it is not planned to support them. But you can still open an issue for it, if there is enough demand for this feature we could try to see if it is feasible.

If you want to be able to use the prop-types related rules (like prop-types or sort-prop-types) you should avoid this pattern for now, sorry :|

@yannickcr yannickcr closed this Jun 5, 2016
@oliviertassinari
Copy link
Contributor Author

oliviertassinari commented Jun 5, 2016

I agree, it's not straightfoward.
Still, we have access to the scope. I think that it can be done in simples cases like this one.
E.g. This is how I'm using the binding with babel: https://github.com/oliviertassinari/babel-plugin-transform-react-remove-prop-types/blob/master/src/index.js#L75.

Sure, I'm gonna open an issue. Thanks.

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

Successfully merging this pull request may close these issues.

3 participants