Skip to content
This repository has been archived by the owner on Sep 21, 2019. It is now read-only.

Keep other static class members, such as defaultProps #36

Merged
merged 5 commits into from
Mar 29, 2018

Conversation

athenayao
Copy link
Contributor

@athenayao athenayao commented Mar 22, 2018

I've been trying this out against our codebase, and I found that it was stripping out all static class members, not just propTypes.

Tracked down the issue to isPropTypesMember where getFullText returns a string surrounded by whitespace; so I've fixed that in 95f6b0e.

That makes it so that we check if propTypes === propTypes, instead of if the propTypes !== propTypes. And that makes it strip out propTypes, but keep all other static class members which is what I think we want!

Now here's the hacky part:
that caused end-to-end initial-state-and-proprypes and end-to-end initial-state-and-proprypes-and-set-state to fail because getFullText returns empty in those cases (Going into the very nitty-gritty, the IdentifierType in these cases has pos: -1, end: -1).

I'm using escapedText here as a fallback but this feels like a gigantic hack; have you run into this before, and do you know what might be the best way to fix it?

@mohsen1
Copy link
Contributor

mohsen1 commented Mar 22, 2018

Thanks for this work. I'll help you to get this merged.

To answer your question, yes. I've seen getText throwing because node is at -1,-1. Guessing this is because node was added by other transformers to the AST without having an actual position in printed source. Copying @RyanCavanaugh and @rbuckton for comments about that.

Also copying @vincentbel to review as he helped a lot with this project as well.

@RyanCavanaugh
Copy link

Guessing this is because node was added by other transformers to the AST without having an actual position in printed source.

This is correct. You can use the function nodeIsSynthesized to determine if a node is synthesized without relying on the underlying representation of -1, -1 to indicate a synthetic node.

What you actually do when faced with a synthetic node is really dependent on the application. Reading the .escapedText is fine IMHO (maybe test using .kind instead of in) but I'd defer to @rbuckton's opinion on that.

@athenayao
Copy link
Contributor Author

.kind looks cool; I've updated to using that entirely instead of getFullText. Since the typedef for SyntaxKind.Identifier shows that escapedText is a required attribute, this seems safe.

@athenayao
Copy link
Contributor Author

Signed CLA!

@@ -97,7 +97,11 @@ export function hasStaticModifier(classMember: ts.ClassElement) {
*/
export function isPropTypesMember(classMember: ts.ClassElement, sourceFile: ts.SourceFile) {
try {
return classMember.name !== undefined && classMember.name.getFullText(sourceFile) !== 'propTypes';
const name =
classMember.name !== undefined && classMember.name.kind === ts.SyntaxKind.Identifier
Copy link
Contributor

Choose a reason for hiding this comment

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

you can use ts.IsIdentifier(classMember.name)

@athenayao
Copy link
Contributor Author

Thanks for the tip. Updated!

@mohsen1 mohsen1 merged commit a2338d9 into lyft:master Mar 29, 2018
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.

3 participants