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

Adding support for objectRestSpread within props #60

Merged
merged 10 commits into from
Feb 20, 2019

Conversation

jnwng
Copy link
Contributor

@jnwng jnwng commented Dec 6, 2017

We hit at an issue properly parsing props that looked like

<div foo={{ params: {...router.params}}} />

because the SpreadProperty doesn't have a key/value pair. Opting to ignore spread properties, but properly recurse over the key/values we can parse.

Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

Seems reasonable

@coveralls
Copy link

coveralls commented Dec 6, 2017

Coverage Status

Coverage increased (+0.02%) to 98.844% when pulling a5b9461 on jnwng:jw/fix-spread-property into e4c0312 on evcohen:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage increased (+0.02%) to 98.844% when pulling a5b9461 on jnwng:jw/fix-spread-property into e4c0312 on evcohen:master.

Copy link
Contributor

@beefancohen beefancohen left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for fixing this!

@coveralls
Copy link

coveralls commented Dec 6, 2017

Coverage Status

Coverage increased (+0.02%) to 98.844% when pulling 26d15ea on jnwng:jw/fix-spread-property into e4c0312 on evcohen:master.

@coveralls
Copy link

coveralls commented Dec 7, 2017

Coverage Status

Coverage increased (+0.02%) to 98.844% when pulling 2eefda3 on jnwng:jw/fix-spread-property into e4c0312 on evcohen:master.

@jnwng
Copy link
Contributor Author

jnwng commented Dec 7, 2017

@evcohen @ljharb thanks for the quick review! as an FYI, this was an issue because eslint-plugin-jsx-a11y is 1) stuck in npm with a dependency on jsx-ast-utils@1.4.0 (not sure what happened there, the code in master is fine and 2) the objectRestSpread errors coming from that.

when this gets merged, would it be possible to bump eslint-plugin-jsx-a11y too? here's the pull request for that one

thank you so much!

@shanecav84
Copy link

shanecav84 commented Feb 20, 2019

@ljharb When can this be merged and released? It fixes #69.

@ljharb ljharb merged commit a1e5007 into jsx-eslint:master Feb 20, 2019
@shanecav84
Copy link

@ljharb Thanks for getting this merged! What's the timeline for a release?

@ljharb
Copy link
Member

ljharb commented Feb 22, 2019

That's up to @evcohen; i don't have publish rights nor am I familiar with the release procedure for this project.

@shanecav84
Copy link

@evcohen When can we get a new release with this PR?

@beefancohen
Copy link
Contributor

Can work on that this weekend! thanks for your patience 😄

@shanecav84
Copy link

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.

5 participants