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

remove quotes when considering the name of a proptype #1132

Merged
merged 2 commits into from May 26, 2017

Conversation

ethanjgoldberg
Copy link
Contributor

@ethanjgoldberg ethanjgoldberg commented Mar 29, 2017

The added test illustrates the failing case. Proptypes with quotes
were referred to by the quoted name, instead of by the bare name,
for flow types.

* @param {string} the identifier to strip
*/
function stripQuotes(string) {
if (string[0] === '\'' || string[0] === '"') {
Copy link
Member

@ljharb ljharb Mar 29, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about backticks?

Copy link
Contributor Author

@ethanjgoldberg ethanjgoldberg Mar 29, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

@ljharb ljharb Mar 29, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Backticked strings require computed property brackets:

{ [`complete?`]: number }

Does this not work with flow?

Copy link
Contributor Author

@ethanjgoldberg ethanjgoldberg Mar 29, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh wild.

It doesn't seem to work with flow in type declarations, and this makes sense to me. Like you say, flow would have to execute the code to compute the key name, since it might have ${}s.

The lint rule also doesn't work for computed property names in normal propTypes: declaration. Maybe something could be done for simple expressions, like backticks without interpolation. But I'm curious if anyone has a good use case for dynamic proptype checking anyway.

Copy link
Member

@ljharb ljharb Mar 31, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will this string always have matching quotes? or would it be better to do an unconditional replace that looks for a starting '/" and a matching token at the end?

Copy link
Contributor Author

@ethanjgoldberg ethanjgoldberg Apr 28, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really can't think of a way to get a key name to start with a quote, without wrapping it in quotes.

I agree it feels a bit hacky. I'll work on an update this weekend.

Copy link
Contributor Author

@ethanjgoldberg ethanjgoldberg May 26, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ljharb, well, it took a bit more than a weekend, but i've updated the function as you suggest.

The added test illustrates the failing case. Proptypes with quotes
were referred to by the quoted name, instead of by the bare name,
for flow types.
@ethanjgoldberg ethanjgoldberg force-pushed the fix-quoted-type-annotations branch from d525a4f to ec94608 Compare May 26, 2017
ljharb
ljharb approved these changes May 26, 2017
Copy link
Member

@ljharb ljharb left a comment

Thanks!

@ljharb ljharb added the bug label May 26, 2017
@ljharb ljharb merged commit ccb213d into jsx-eslint:master May 26, 2017
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

None yet

2 participants