[WIP] Refactor: Convert React.Component, React.SFC, React.PureCompoennt to each other#24518
[WIP] Refactor: Convert React.Component, React.SFC, React.PureCompoennt to each other#24518Jack-Works wants to merge 8 commits intomicrosoft:masterfrom Jack-Works:feature/refactor-react-component
Conversation
…toryIsReact; Rewrite isConvertibleReactClassComponent
| "category": "Message", | ||
| "code": 95057 | ||
| }, | ||
| "Covert React.Component to SFC": { |
There was a problem hiding this comment.
nit. Convert component to a Stateless Functional Component (SFC)
| "category": "Message", | ||
| "code": 95058 | ||
| }, | ||
| "Covert React.SFC to PureComponent": { |
There was a problem hiding this comment.
Convert Stateless Functional Component to a statefull component
There was a problem hiding this comment.
@DanielRosenwasser can you help out with these messages..
There was a problem hiding this comment.
Is Pure Component better than stateful component?
Refactor provides an action to PureComponent but not Component, because PureComponent has better performance in default, if developers want to use state, they can just remove Pure.
| } | ||
| }, | ||
| getEditsForAction(context: RefactorContext, actionName: string): RefactorEditInfo | undefined { | ||
| Debug.assert(actionName === actionNameComponentToSFC || actionName === actionNameSFCToPureComponent); |
There was a problem hiding this comment.
you want to check again for the same conditions to defensively guard against changes to the file state before the actions are requested.
I would extract the code in getAvailableActions to getConvertableReactElemnt that will return you the SFC/Class or undefined if not found.
There was a problem hiding this comment.
See generateGetAccessorsAndSetAccessors.ts for a sample.
| StatelessComponent: undefined, | ||
| } as any; | ||
| let importClause: ImportClause; | ||
| sourcefile.forEachChild(c => { |
There was a problem hiding this comment.
i do not think we should be walking the file as such. first there are easier ways to get the resolved module references, and second, this does not handle cases where anything is aliased.. e.g. reactNamespace, jsxFactory, path mapping in jsconfig.json.. etc..
I would base all the checks on the class/function, and then verify that whatever its import clause is coming from the same location as the JSXFactory.
There was a problem hiding this comment.
I can't get it, about the "easier ways to get the resolved module". I look around for the compiler API, there seemed to be a SourceFile.getNamedDeclarations can get me back all declarations. And in runtime I found it has a property called propertyName can give me the import declarations and its original name. But there is no such type on the type Declaration, it seemd not like the correct way to do it.
And, by using SourceFile.getNamedDeclarations, there is not much difference with I iterate over all children.
| const { file } = context; | ||
| const span = getRefactorContextSpan(context); | ||
| const token = getTokenAtPosition(file, span.start, /*includeJsDocComment*/ false); | ||
| const component = getParentNodeInSpan(token, file, span); |
There was a problem hiding this comment.
this will enable the refactor in any location within the declaration. we want to limit it to spans that include the name of the declaration. so first get the token, walk up untill you find a class/class expression or a function/function expression/lambda, verify that the name of the class, or function keyword or class keyword or the name of the const/var on the left hand side is included in the span.
There was a problem hiding this comment.
see generateGetAccessorAndSetAccessor.ts:: getConvertibleFieldAtPosition for an example.
|
|
||
| // function checkJSXFactoryIsReact(host: LanguageServiceHost) { | ||
| // const config = host.getCompilationSettings(); | ||
| // if (config.jsxFactory === undefined) { if (config.jsx) return true; } |
There was a problem hiding this comment.
as i noted erlier, you do not want to limit this to React or the module name "react" there is peact, vue, mithril, etc..
There was a problem hiding this comment.
But this refactor is meaningless for the Vue and Mithril, why don't we limit it to React, ReactNative and Preact?
| if (!node.body) { return; } | ||
|
|
||
| // TODO: Should also check the actual type insteadof only receive explicit typed | ||
| const typeNode = node.type; |
There was a problem hiding this comment.
we should not be relying on type annoation. tsserver support JS experience as well. so these apply to both JS and TS files.
we should isntead try to "guess" if this is an SFC or not..
- funciton declaration
- or const declaration with initializer function expression or lambda
- Zero, one or two parameters
- function returnns a JSX element (this is gonna be a bit tricky, but we can start by saying the expression of all return statements must be a JSX element).
There was a problem hiding this comment.
We can go further and say it has to be at the top-level of the file.
There was a problem hiding this comment.
Yes, in todo I'll try to guess if it is an SFC after I done the explicit typed SFC.
context, propTypes, contextTypes, defaultProps and displayName are also in later stage of pr.
There was a problem hiding this comment.
And, hmm, what does it mean by at the top-level of the file? Like it is one of the SourceFile's children?
| render = node.body; | ||
| } | ||
| else if (isExpression(node.body!)) { | ||
| render = createBlock([createReturn(node.body)]); |
There was a problem hiding this comment.
do not create a block untill you are applying the edit.
There was a problem hiding this comment.
I got a problem with manipulating the AST and get the textchange, do I do something wrong? Thanks
| } | ||
|
|
||
| function isReactSFCDeclaration(node: Node, sourcefile: SourceFile): Component | undefined { | ||
| if (!isSFCLikeDeclaration(node)) { return; } |
There was a problem hiding this comment.
combine in one if statement
|
|
||
| function isReactSFCDeclaration(node: Node, sourcefile: SourceFile): Component | undefined { | ||
| if (!isSFCLikeDeclaration(node)) { return; } | ||
| if (node.asteriskToken) { return; } |
There was a problem hiding this comment.
return undefined explicitly
| const newNode: FunctionDeclaration = createFunctionDeclaration( | ||
| void 0, void 0, void 0, component.name, void 0, | ||
| [createParameter(void 0, void 0, void 0, "props", void 0)], | ||
| createTypeReferenceNode(react.SFC, component.propsType ? [component.propsType] : undefined), |
There was a problem hiding this comment.
When I tested this, react.SFC was the string "React.Component". That's not a valid identifier, so it causes assertion failures in the formatter (#24751). Should use an EntityName node instead.
Also, when a node in the old tree is moved to the new tree, you should use getSynthesizedDeepClone to get new nodes.
There was a problem hiding this comment.
Thanks, with your help I can continue working on it
|
Due to new concepts in React@next (about 16.7), the complexity of this refator has been more higher. Since the develop has been paused for months and it's out of date, so I decided to close this pull request. |
|
I presume the new concepts is https://reactjs.org/docs/hooks-intro.html ? |
|
Yes, and also React.memo( ) |
Related to #15090
This PR adds a refactor to convert SFC-like function to PureComponent or Component to SFC.
This will help one who wants to add lifecycle methods to an SFC component.
Example
To-dos
Behavior in detail