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

Support JSX Fragment syntax for jsx-wrap-multiline. #130

Merged
merged 3 commits into from
Dec 20, 2017

Conversation

dmiller9911
Copy link
Contributor

Fixes #124

Added a check for ts.syntaxKind.JsxFragment in jsx-wrap-multiline. A newer version of tsutils has this check as well, but I was unsure of the policy for updating dependencies. If you would rather upgrade I will do that instead.

Add tests to verify positive and negative cases. Upgrade ts devDependency to 2.6.2.

@palantirtech
Copy link
Member

Thanks for your interest in palantir/tslint-react, @dmiller9911! Before we can accept your pull request, you need to sign our contributor license agreement - just visit https://cla.palantir.com/ and follow the instructions. Once you sign, I'll automatically update this pull request.

elementClose: ts.LineAndCharacter,
closingTag?: ts.JsxClosingElement,
) {
attributes = Array.isArray(attributes) ? attributes : attributes.properties;
if (attributes.length === 0) {
const attrs = Array.isArray(attributes) ? attributes : attributes.properties;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

TS 2.6.2 did not like the reassignment of attributes. Setting to a new variable and adding the typing resolved the type checks.

@adidahiya
Copy link
Contributor

A newer version of tsutils has this check as well, but I was unsure of the policy for updating dependencies. If you would rather upgrade I will do that instead.

Yes please, feel free to upgrade tsutils

Copy link
Contributor

@adidahiya adidahiya left a comment

Choose a reason for hiding this comment

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

lgtm aside from the suggestion to take this from tsutils.

@@ -34,6 +34,6 @@
"path": "^0.12.7",
"tslint": "^5.5.0",
"tslint-language-service": "^0.9.6",
"typescript": "~2.4.2"
"typescript": "~2.6.2"
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@dmiller9911
Copy link
Contributor Author

Will wait for ajafff/tsutils#17 before updating tsutils.

@dmiller9911 dmiller9911 force-pushed the jsx-wrap-multiline-fragments branch from 85c6931 to f3d49ee Compare December 14, 2017 16:36
@adidahiya
Copy link
Contributor

@dmiller9911 can you please pull in master to get the new CI setup and required checks? Also enable CI on your branch if you haven't yet.

Add tests to verify positive and negative cases. Upgrade ts devDependency to 2.6.2.
@dmiller9911 dmiller9911 force-pushed the jsx-wrap-multiline-fragments branch from f3d49ee to 20ed872 Compare December 14, 2017 21:15
@adidahiya adidahiya merged commit 29d1330 into palantir:master Dec 20, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants