Add support for template codemods #81

simonihmig · 2020-05-29T14:00:33Z

Closes #8

bin/cli.js

simonihmig · 2020-05-29T14:07:04Z

So tests were passing for me locally. This fails in CI due to:

ember-template-recase requiring node 10+. Which we could rely when Drop Node 8 support. #76 is merged
but that PR and even the master branch is failing CI, due to somehow execa not being able to execute yarn in a test. Idk why, briefly tried to fix this, e.g. by upgrading execa, but that didn't help either...

@rwjblue any clue what's going on?

rwjblue · 2020-06-06T21:14:23Z

No, not sure. I was also having similar issues, and then ran out of time 😢.

I need to try to revive my Node version bump PR and figure out what is up 🤔

simonihmig · 2020-06-25T08:33:17Z

After rebasing, this is green now, so waiting for your review @rwjblue! 🙂

Btw, this is already in use (through my github fork) in https://github.com/kaliber5/ember-bootstrap-codemods...

commands/local/generate/codemod.js

commands/local/generate/fixture.js

src/bin-support.js

src/test-support.js

src/test-support/template.js

…ather than visitor fn

As ember-template-recast handles that, rwjblue#81 (comment)

A generated transform now exports its own type. This allows us to * remove the `type` arguments from `runTransform()`, which was problematic anyway as that type would have to passed in the project's `cli.js`, so would not have allowed to have different transform types per project. Instead the type is taken from the transform's exported type itself. * same for `runTransformTest()`: `options.type` is not needed anymore, as the type is also taken from the transform itself * instead of failing when a user tries to generate a fixture for a transform with non-matching types, the generate fixture command now implicitly determines the fixture type from its transform type

simonihmig · 2020-07-10T18:12:15Z

I refactored the previous implementation quite a bit, inspired by your feedback. A generated transform now exports its own type. This allows us to

remove the type arguments from runTransform(), which was problematic anyway as that type would have to passed in the project's cli.js, so would not have allowed to have different transform types per project. Instead the type is taken from the transform's exported type itself.
same for runTransformTest(): options.type is not needed anymore, as the type is also taken from the transform itself
instead of failing when a user tries to generate a fixture for a transform with non-matching types, the generate fixture command now implicitly determines the fixture type from "its" transform type

I hope this makes sense, @rwjblue please re-review!

rwjblue

That looks great, thanks for doing that refactor!

rwjblue · 2020-07-14T17:13:28Z

commands/local/generate/fixture.js


  let { codemodName, fixtureName } = options;
  let codemodDir = `${process.cwd()}/transforms/${codemodName}`;
+  let codemodTransform = `${codemodDir}/index.js`;


Do we need to add index.js here? I wonder if we can basically just let it fall through via the require that is done down in getTransformType (since require(codemodDir) and require(codemodDir + '/index.js') are effectively the same thing)? That would allow folks to use other extensions (for example .ts)...

rwjblue · 2020-07-14T17:15:09Z

src/bin-support.js

+function getTransformPath(binRoot, transformName) {
+  const path = require('path');
+
+  return path.join(binRoot, '..', 'transforms', transformName, 'index.js');


Can we do require.resolve(path.join(binRoot, '..', 'transforms', transformName))?

rwjblue · 2020-07-14T17:27:26Z

src/test-support/template.js

@@ -0,0 +1,67 @@
+'use strict';


It seems to me that this is (or could be) basically the same as the jscodeshift one but we run a different function for actually testing (jscodeshift runs runInlineTest and this one runs runTemplateTest).

I would prefer to refactor these together to avoid the duplication that exists right now, and to also ensure that both support the same feature set. At the moment, I think this file is missing the ability to test passing different command line options (that the template transform can access with getOptions).

Makes sense, done.

rwjblue · 2020-07-14T17:27:56Z

src/test-support/utils.js

+  let root = process.cwd() + `/transforms/${options.name}/`;
+  let transformPath = root + 'index.js';


Seems like this should be using getTransformPath

ok, I now put getTransformPath into transform-support.js, and changed the signature a tiny bit, to expect the project's root folder (instead of binRoot, as we don't have/need binRoot here).

rwjblue · 2020-07-14T17:29:06Z

src/transform-support.js

+  if (!fs.existsSync(transformPath)) {
+    throw new Error(`Transform ${transformPath} not found.`);
+  }


If we use require.resolve in getTransformPath then I think this can be removed (since require.resolve would throw)

rwjblue · 2020-07-15T01:30:09Z

Thank you very much @simonihmig! I'll try to get a release out in the morning...

Updated for the further changes to codemod-cli landed in rwjblue/codemod-cli#81 after review. (still waiting for an official release though) Fixes #5

simonihmig changed the title ~~Add support for Template support~~ Add support for template codemods May 29, 2020

simonihmig commented May 29, 2020

View reviewed changes

bin/cli.js Outdated Show resolved Hide resolved

simonihmig mentioned this pull request May 29, 2020

Add support for template-recast codemods #8

Closed

simonihmig force-pushed the template-support branch from 954ad68 to 0d3607e Compare June 24, 2020 20:10

rwjblue added the enhancement New feature or request label Jun 24, 2020

rwjblue requested changes Jun 25, 2020

View reviewed changes

simonihmig added 6 commits July 10, 2020 20:07

Add template test support

b293927

Add bin-support for templates, updated test-support for full plugin r…

c809c5d

…ather than visitor fn

Add generator for template transforms and tests

7995cbf

Address PR feedback

9dba17c

Remove extra globby call in bin-support/runTemplateTransform

6bf475a

As ember-template-recast handles that, rwjblue#81 (comment)

simonihmig force-pushed the template-support branch from 3c734ca to 1902c28 Compare July 10, 2020 18:08

rwjblue reviewed Jul 14, 2020

View reviewed changes

Cleanup after PR feedback

a21d5ca

rwjblue approved these changes Jul 15, 2020

View reviewed changes

rwjblue merged commit 1e08083 into rwjblue:master Jul 15, 2020

simonihmig added a commit to ember-bootstrap/ember-bootstrap-codemods that referenced this pull request Aug 4, 2020

Update codemod-cli

fb3e3ae

Updated for the further changes to codemod-cli landed in rwjblue/codemod-cli#81 after review. (still waiting for an official release though) Fixes #5

simonihmig mentioned this pull request Aug 4, 2020

Update codemod-cli ember-bootstrap/ember-bootstrap-codemods#6

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for template codemods #81

Add support for template codemods #81

simonihmig commented May 29, 2020

simonihmig commented May 29, 2020

rwjblue commented Jun 6, 2020

simonihmig commented Jun 25, 2020

simonihmig commented Jul 10, 2020

rwjblue left a comment

rwjblue Jul 14, 2020

simonihmig Jul 15, 2020

rwjblue Jul 14, 2020

simonihmig Jul 15, 2020

rwjblue Jul 14, 2020

simonihmig Jul 15, 2020

rwjblue Jul 14, 2020

simonihmig Jul 15, 2020

rwjblue Jul 14, 2020

simonihmig Jul 15, 2020

rwjblue commented Jul 15, 2020

		let root = process.cwd() + `/transforms/${options.name}/`;
		let transformPath = root + 'index.js';

Add support for template codemods #81

Add support for template codemods #81

Conversation

simonihmig commented May 29, 2020

simonihmig commented May 29, 2020

rwjblue commented Jun 6, 2020

simonihmig commented Jun 25, 2020

simonihmig commented Jul 10, 2020

rwjblue left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rwjblue commented Jul 15, 2020