Skip to content
This repository has been archived by the owner on Mar 25, 2021. It is now read-only.

Add JSX text nodes into skip list of walker (fixes #2007) #2009

Closed
wants to merge 3 commits into from

Conversation

IllusionMH
Copy link
Contributor

@IllusionMH IllusionMH commented Jan 8, 2017

PR checklist

What changes did you make?

This PR adds JsxText nodes into skip map in SkippableTokenAwareRuleWalker.

Is there anything you'd like reviewers to focus on?

  1. This change should definitely skip JsxText nodes in EnableDisableRulesWalker, CommentWalker and JsdocWalker, but I'm unsure about WhitespaceWalker and NoTrailingWhitespaceWalker.

  2. If JsxText node consist of whitespace and "comment" it isn't added to skip map since getStart() and getEnd() return same value. See test case

@ajafff
Copy link
Contributor

ajafff commented Jan 8, 2017

I'm unsure about WhitespaceWalker

WhitespaceWalker skips Jsx(Selfclosing)Element anyways, so skipping JsxText may not be a big deal here. I don't know why they are skipped though. Maybe to solve a similar problem like this PR?

@IllusionMH
Copy link
Contributor Author

IllusionMH commented Jan 8, 2017

WhitespaceWalker most likely won't be affected by this change, but NoTrailingWhitespaceWalker might have some logic here and will require separate implementation.
Looks like NoTrailingWhitespaceWalker already reported trailing whitespaces on JSX text nodes so I restored its functionality. But implementation isn't very nice.

Still there is a question: if there is need to support text nodes that consist of whitespaces and text that matches comment format?

@@ -41,6 +41,11 @@ export class Rule extends Lint.Rules.AbstractRule {
}

class NoTrailingWhitespaceWalker extends Lint.SkippableTokenAwareRuleWalker {
// prevents skipping of JsxText nodes
protected visitJsxText(node: ts.JsxText) {
this.walkChildren(node);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is there other way to restore super.super.visitJsxText functionality?

Copy link
Contributor

Choose a reason for hiding this comment

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

how about adding a constructor to SkippableTokenAwareRuleWalker with a parameter skipJsx?

@ajafff
Copy link
Contributor

ajafff commented Jan 10, 2017

Maybe it would be sufficient to just pass the correct ts.LanguageVariant to ts.createScanner. Currently on LanguageVariant.Standard is used, so the scanner doesn't know how to parse jsx. In this case LanguageVariant.JSX is needed.
The easiest way would be to use sourceFile.languageVariant.

@IllusionMH
Copy link
Contributor Author

IllusionMH commented Jan 12, 2017

@ajafff Thanks for suggestion.
I tried to use sourceFile.languageVariant as well as LanguageVariant.JSX in scanner initialization, and haven't found any difference at first glance.

I'll try to dive deep into TS scanner difference between language variants and other ways to handle text nodes that has only comment text.

@IllusionMH
Copy link
Contributor Author

Fixed in #2036.

@IllusionMH IllusionMH closed this Jan 16, 2017
@nchen63
Copy link
Contributor

nchen63 commented Jan 16, 2017

superseded by #2036

@IllusionMH IllusionMH deleted the visit-jsx-text branch January 16, 2017 19:30
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants