Add support for chained gets, sets, and methods #69
Conversation
There was a problem hiding this comment.
Overall looks good but I'd like to get that particular tricky case to be a bit more self-documenting.
Is there no need to do this same kind of recursive application for the other mods? For example what about new Document(new String('foo')) or something like that?
lib/mods/replace-get-set.js
Outdated
| @@ -48,7 +48,8 @@ function isDataPropertyAccess(node) { | |||
| */ | |||
| function isGlobalPropertyAccess(node) { | |||
| return nodeTypes.isPropertyAccessExpression(node) && | |||
| !nodeTypes.isCallExpression(node.getParent()) && | |||
| !(nodeTypes.isCallExpression(node.getParent()) && | |||
| node.getParent().getExpression() === node) && | |||
There was a problem hiding this comment.
Is it possible to express what this condition is asking? e.g.
const ___something___ = nodeTYpes.isCallExpression(node.getParent()) && node.getParent().getExpression === node;and then just use !___something__? Where __something__ is a semantic name indicating what actually you're testing for...
There was a problem hiding this comment.
I was considering documenting via comments but this seems like a good way to do it, I'll change to this
There was a problem hiding this comment.
Updated to a separate var with a explanatory comment
|
We only need the recursive application for methods and gets/sets, since, as of a few PRs ago, constructors don't rewrite. It's also unnecessary for the remove console mod as that only rewrites to |
I still haven't figured out how this change works. If recursion only happens within a module, how does document.body.querySelector('class');get rewritten correctly, that seems like we have to recurse into 2 modules. Maybe I don't understand how the tree traversal works at all. |
|
I did some digging and that particular example works, but there is still an issue. document.body.querySelector('class');This would first have the Reflect_apply(HTMLElement_querySelector, document.body, ['class']After the Reflect_apply(HTMLElement_querySelector, Reflect_apply(Document_body_get, document), ['class']);After this Reflect_apply(HTMLElement_querySelector, Reflect_apply(Document_body_get, Window_document_get()), ['class']); |
|
This example works, but only because of the order of the accesses, with method coming before gets/sets. If, for example, we had something closer to: const otherDiff = document.querySelector('body').textContent;Then the Reflect_apply(Document_querySelector, Window_document_get(), ['body']).textContent;This is an issue, and will likely require rethinking the strategy of only recursing with the current method. |
Resolves #29