Integration with Putout code transformer #30
Conversation
|
There was some techincal reason I called recast directly in this weird way, because normally I would use jscodeshift in the normal way, but I forget what wasn't working. I'll look into it. How come it wouldn't work as-is with putout though? If running with stock jscodeshift, a transform can process a file in any way it wants, it just has to return a string... |
|
Oh now i remember. When I wrote this there were too many bugs in the version of |
|
|
But if the transform parses the code on its own it returns a string just like any other jscodeshift transform, can putout not use that returned string? If not, then putout is not technically 100% supporting all jscodeshift transforms. I would need a system to detect the version of jscodeshift that's injected and error out if it's too old so that users don't think codemod glitches are due to this package itself. |
|
Also transforms can completely sidestep jscodeshift's parser dependency injection by doing exports.parser = { parse: (code) => ... }Which I guess would be another way I could make it use the version of recast I want... |
There is no such target, but the cases written in jscodeshift documentation is supported. What I want to suggest you is using module.exports.replace = () => ({
'__a(__args).then(__b)': '__b(await __a(__args))'
})It will transform: fetch('/api.json').then(parse);to: parse(await fetch('/api.json')); |
|
I've been working on my own tool called When it comes to transforming promises to async/await though, I understand duplicate parsing would be slow, but if I do it the way you suggest, how can I guarantee that the code is parsed with a recent-enough version of |
It looks very good :). Very laconic syntax, the only thing is tests count very small.
You absolutely right, and
I think mention supported version of jscodeshift in documentation will be enough.
Every
What if new developer, that just joined team didn’t know all the rules related to promises in project. He writes code, and then his IDE helps him to fix it with autofix using putout integration with eslint. I think it's very useful :). |
I guess but that raises the likelihood that I have to deal with issues raised by someone who didn't read that...
We may think it's foolish not to, but no JS project has to use a formatter. It's nice to be able to not change the format of code that a codemod doesn't touch, so that it doesn't cause problems for people who aren't using formatters. Using
I don't know if you saw that most of my tests are organized as fixture files, test coverage isn't as high as yours but here's the coverage Does
Sure, I'm just saying, I really don't think people will be running |
|
Now that i think about it though, we could do a compromise solution: I could add another entry module that parses using the jscodeshift api, so you can call that one |
|
The eslint folks were interested in working on an async-await autofix at one point too, but asyncify is invasive enough that I think it should be used with caution, rather than an always-autofix manner |
|
Plus, it can still mess up comments on any version of recast. So dangerous to use automatically all the time |
exports.find = `{ $_before, foo: 1, $_after }`
exports.replace = `{ $_before, foo: 1, rest: { $_after } }`It will be something like this: That's right,
Look, This is after calling
That's right, the more cases we will receive, the better quality of a |
Yeah its output can be goofy sometimes. But the main codebase I made asyncify for, That's cool that you made a putout IDE with AST Explorer. If putout can do enough things that astx can do I might switch to it. Can it match a series of statements, or just single statements and expressions? Can replace functions access the captured nodes and paths, or can they only be used in replacement strings? |
|
One of the most involved refactorings I did was exports.astx = ({ astx, statements, j }) => {
astx.findStatements`
const code = $code
const root = j(code)
const result = addImports(
root,
$imports
)
expect(result).to.deep.equal($expectedReturn)
expect(root.toSource()).to.equal($expectedCode)
`.replace(({ captures: { $imports } }) => {
if ($imports.quasi)
return statements`
testCase({
code: $code,
add: ${j.stringLiteral($imports.quasi.quasis[0].value.raw)},
expectedCode: $expectedCode,
expectedReturn: $expectedReturn,
})
`
})
}(It sounds like you have a convenient syntax for capturing quasis inside template string literals which is great, but I can't quite tell from the readme how that works) Which made a bunch of changes like - it(`adds missing non-default requires without alias`, function() {
- const code = `const {bar} = require('baz')`
- const root = j(code)
- const result = addImports(
- root,
- statement`const {foo: qux} = require('baz')`
- )
- expect(result).to.deep.equal({ qux: 'qux' })
- expect(root.toSource()).to.equal(`const {
- bar,
- foo: qux
-} = require('baz')`)
+ it(`adds missing non-default requires without alias`, function () {
+ testCase({
+ code: `const {bar} = require('baz')`,
+ add: "const {foo: qux} = require('baz')",
+ expectedCode: `const {
+ bar,
+ foo: qux
+ } = require('baz')`,
+ expectedReturn: { qux: 'qux' },
+ })
}) |
|
It's also worth noting that
It was okay to leave Also, sometimes it's a matter of personal preference whether to use |
|
Okay I added an entrypoint that you can use. Or you can use the https://github.com/codemodsquad/asyncify/blob/master/README.md#disabling-recast-workaround |
|
I'd still discourage users from using this in a routine, eslint autofix kind of way, for reasons described above. But you and some eslint contributors seem to think that would be worthwhile, I just want to make sure it's clear that there are risks to it. |
For const {operator, template} = require('putout');
const {compare, getTemplateValues} = operator;
const patterns = [
'const code = __a',
'const root = j(code)',
'const result = addImports(root, __b)',
'expect(result).to.deep.equal(__c)',
'expect(root.toSource()).to.equal(__d)',
];
const buildTestCase = template(`
testCase({
code: CODE,
add: ADD,
expectedReturn: EXPECTED_RETURN,
expectedCode: EXPECTED_CODE,
})
`);
const getValue = (body, position) => {
const currentNode = body[position];
return getTemplateValues(currentNode.expression || currentNode, patterns[position]);
};
module.exports.match = () => ({
'it(__a, __b)': ({__b}, path) => {
const {body} = __b.body;
for (let i = 0; i < patterns.length; i++) {
const pattern = patterns[i];
const currentNode = body[i];
if (!compare(currentNode, pattern))
return false;
}
return true;
}
});
const CODE_POSITION = 0;
const ADD_POSITION = 2;
const EXPECTED_RETURN_POSITION = 3;
const EXPECTED_CODE_POSITION = 4;
module.exports.replace = () => ({
'it(__a, __b)': ({__b}, path) => {
const {body} = __b.body;
const {__a: CODE} = getValue(body, CODE_POSITION);
const {__b: ADD} = getValue(body, ADD_POSITION);
const {__c: EXPECTED_CODE} = getValue(body, EXPECTED_RETURN_POSITION);
const {__d: EXPECTED_RETURN} = getValue(body, EXPECTED_CODE_POSITION);
const ast = buildTestCase({
CODE,
ADD,
EXPECTED_CODE,
EXPECTED_RETURN,
});
__b.body.body = [
ast,
];
return path;
}
});Little bit more code, but most of the time I'm working with short patterns, 5 lines of code with absolutely same name and meaning it's a rare case, anyways, it is possible :).
That's what I'm telling about, you got a lot expertise in converting |
|
I see, yeah To that end, the Ideally, some day I'll figure out how to make a VSCode extension that provides a frontend UI for |
|
I see that astx very powerful, would be amazing to use it in
compare('const a = `hello`', 'const __a = `__b`');
compare(node, 'const __a = `__b`');It seems to me that if we combine efforts, we can get a universal tool for working with AST that will significantly outperform existing solutions. What I'm thinking most of the time is improving and simplifying ecosystem of plugins. Here is similar project for Current implementation of |
|
Here is simpler solution :) const from = `
it(__a, function() {
const code = __b;
const root = j(code);
const result = addImports(root, __c);
expect(result).to.deep.equal(__d);
expect(root.toSource()).to.equal(__e);
});
`;
const to = `
it(__a, function() {
testCase({
code: __b,
add: __c,
expectedReturn: __d,
expectedCode: __e,
});
})
`;
module.exports.replace = () => ({
[from]: to,
}) |
|
Ah yes. It's kind of like how it's difficult to match just an object property, syntactically you have to write the object around it. Though I've been thinking of making a special |
|
By the way, since this package mostly uses the babel API instead of ast-types, I could try to make an entry point that doesn't use recast at all if you're interested. It might have less comment issues, because Recast uses the |
|
Yeah I've thought about making a babel version of astx as well. And yeah I still need to publish it, need to finalize and document some aspects of the CLI and transform file format. It might be hard for us to combine efforts because I can be opinionated :P but we are after a lot of the same things so we should at least learn about each other's approaches. After some false starts with my compare implementation I refactored it into a |
|
Your implementation of Passing the Being able to have the For example, with the query (template): import { $_a, foo } from 'foo'The (Note: there's no backreferencing of array slice captures like The same happens with a type parameters, for example: class A<$_a, foo> { }In this case the In astx, |
|
@coderaiser Okay I just finally added some basic documentation for the CLI and transform file format and published |
|
I made nice find output for the CLI:
|
|
Here is what I came up with coderaiser/putout@5846c23#diff-ef2420e6e56a8db4df7bf8c4fe146d4bf4d834cfa0531848d7e4c1ea09d83dedR33-R66 export interface CompiledMatcher {
predicate?: false
captureAs?: string
arrayCaptureAs?: string
match: (path: ASTPath<any>, matchSoFar: MatchResult) => MatchResult
nodeType?: keyof typeof t.namedTypes | (keyof typeof t.namedTypes)[]
}This looks little bit similar to |
|
No idea really, I've never looked at regex implementations much. I do wonder how my approach to array matching with variable-length placeholders compares to how various regex implementations work |
|
@coderaiser I just ran into that issue with I still think being able to reprint a modified AST and preserve formatting of untouched nodes is a worthwhile goal, but the way recast does it seems like a mess... Btw, I noticed another tool similar to our stuff called So it's interesting, this is a burgeoning space and it seems like collective consciousness is developing about the need for structural search and replace. But I think there's still a need for tools dedicated to JS. For example semgrep doesn't support Flow syntax right now, and I doubt it will ever support all the different optional syntaxes people can configure in Babel, like the pipeline proposals (I made I've been rewriting my astx replacement engine to be really smart about transmuting captured nodes to fit in the replacement context. For example: // before
const foo = require('foo')
const bar = require('bar')
const {glom, qlx} = require('foo')
const {bar: barr, default: qux} = require('bar')
// -f 'const $1 = require("$2")' -r 'import $1 from "$2"'
// after
import foo from 'foo'
import bar from 'bar'
import {glom, qlx} from 'foo'
import qux, {bar as barr} from 'bar'This is the kind of JS-specific attention to detail I think cross-language tools will probably never find time to implement. I'm also planning to change my array matching syntax to use After I finish this I'll probably either convert it to pure Babel or figure out a way to make pluggable jscodeshift/Babel backends. |
Looks good, I'll test it and tell what it can find: === setting up agent configuration
| using 652 semgrep rules configured on the web UI
| using 477 code asset inventory rules
| using default path ignore rules of common test and dependency directories
| found 2644 files in the paths to be scanned
| skipping 474 files based on path ignore rules
=== looking for current issues in 2170 filesWhat I see right now: it doesn't transform, only search without progress bar, who knows when it's done 🤷♂️. It's finished! Which is made for a reason, so it's not very useful findings. By the way, have you seen my PR?
I agree, I think a lot JS-specific features will be missed in such tools. About |
|
My point isn't just about creating the ability to convert requires to imports but that the transmutation opens up new possibilities and ease of transformation for unanticipated use cases. For example say for whatever reason you wanted to refactor const {default: foo, bar: baz} = loadStuff()To import foo, {bar as baz} from 'stuff'Or vice versa. Niche transformations like this are trivial to do with the new replacement engine compared to previous approaches. Cases like this may seem kind of unlikely, but I believe this will prove surprisingly useful for unanticipated use cases, because refactoring use cases can vary wildly. So my goal is to make a system so powerful and flexible that I'm able to accomplish transformations I can't even anticipate at the moment with it. I think a lot of linting rules for obvious mistakes like Another thing I see a need for is using one pattern to capture where an identifier gets bound to scope, and then doing a find/replace on patterns that refer to that same identifier binding, rather than any identifier with the same name. Because doing scope lookups with recast/babel APIs is still tedious. I haven't decided what the API for finding scoped bindings with patterns would look like yet though. Semgrep is weird to me because I can't tell if the pattern language has a precise grammar for how capture variables and |
This is a good goal :) and
So I’m working on practical things that improves JavaScript code, not abstract things that I can't imagine. If I trap in case when I have to do the task of capturing arrays of nodes and interpolating them in replacements more often then never I’ll implement such things. All
I’m OK with Babel API for work with scopes, but I understand what are you talking about. I’m also have no idea how it can be expressed in declarative way (especially with keeping in mind compatibility with
That’s true, but video presentation really interesting, this guy made a lot of work building universal AST for so many languages. What I understand well, that if you doing linter for a language then you should definitely program on it every day, in other case you just want trap on a lot issues that every programmer trap and that can be fixed. Same go’s for frameworks, to build a good quality tool it should be used a lot. |
You can integrate easily with putout code transformer, and
asyncifycan be used as a plugin, similar to async-await-codemod.A little change is required so code written in the way that jscodeshift suggests.