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

Nested accessors and methods #29

Open
jackbsteinberg opened this issue Aug 8, 2019 · 3 comments · Fixed by #69
Open

Nested accessors and methods #29

jackbsteinberg opened this issue Aug 8, 2019 · 3 comments · Fixed by #69
Milestone

Comments

@jackbsteinberg
Copy link
Owner

jackbsteinberg commented Aug 8, 2019

Currently when the rewriter encounters nested accessors and methods it will only convert the outermost to its original:

const pathLength = url.pathname.length;
const expressionName = node.getExpression().getSymbol().getName();

---- REWRITTEN ----

const pathLength = Reflect_apply(String_length_get, url.pathName);
const expressionName = Reflect_apply(Symbol_getName, node.getExpression().getSymbol(), []);

This is possibly because parents of observed nodes are being rewritten, which prevents the algorithm from continuing to inspect and properly rewrite all the children.

@domenic
Copy link
Collaborator

domenic commented Aug 8, 2019

Solutions discussed:

  • First, refactor all codemods so that you never replace the parent node. This might magically solve the problem, depending on the implementation details of ts-morph's forEachDescendant().

  • If that does not solve the problem, then implement our own depth-first postorder traversal to replace forEachDescendant(). This depends on the previous step; a depth-first postorder traversal will fail badly if we're replacing the parents of nodes that are in the process of being traversed.

@domenic
Copy link
Collaborator

domenic commented Aug 19, 2019

This may be more complicated than just applying the transforms recursively, because the first transform could cause a loss of type information. See #35 (comment) for a similar issue and a proposed solution.

@fergald
Copy link
Collaborator

fergald commented Sep 10, 2019

Reopening based on #69 (comment)

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 a pull request may close this issue.

3 participants