refactor(remix-dev): rewrite `jscodeshift`-based migrations as `babel`-based codemods #4572

pcattori · 2022-11-11T04:50:08Z

Summary

Removes the convert-to-javascript migration from the CLI since it was never intended to be user-facing and was undocumented
- but for now, keeps the convert-to-javascript code for use in create command when users opt out of TS
- Until we deprecated and remove TS->JS conversion, we'll still have jscodeshift as a dependency in @remix-run/dev
Renames the replace-remix-imports ~~migration~~ codemod to replace-remix-magic-imports for clarity
Rewrites the replace-remix-magic-imports codemod
- Removes need for network connection
- Uses the Babel Visitor API which is simpler and more ubiquitous than jscodeshift API
  - Babel's Visitor API can be wrapped to provide a jscodeshift transform if needed in the future
  - jscodeshift spun up workers to process file transformations, which had significant overhead and is why some Windows CI tests were flakey
Renames "migrate"/"migration" to "codemod" to disambiguate from database migrations
Adds a CLI testing utility
- captures stdout and stderr for less noise when running tests
- uses the built cli.js when it is up-to-date with respect to the source cli.ts for 2-5x faster tests
Restores the replace-remix-magic-imports fixture to represent a Remix 1.3.x project
The replace-remix-magic-imports codemod TUI is simplified to use ora spinners/✔️/𐄂 and runs completely without need for interactive user input

Results

The net effect is that codemod tests run 10-100x faster.
For example the replace-remix-magic-imports codemod test used to take ~30-35s.
Now, the same test takes 0.8s when using build is up-to-date, and 2.5s when build is out-of-date.

This codemod does not use a network connection and does not spin up workers, increasing speed and reliability, especially for Windows CI tests.

BEFORE

AFTER

Testing strategy

Added tests for transform and codemod:

Added: codemod-test.ts: check that project is a clean git repo and the specified codemod exists
Improved: codemod-replaceRemixMagicImports-test.ts: check that remix package is removed, dependencies are updated, and remix setup is removed from postinstall script
Added: transform-replaceRemixMagicImports-test.ts: check that import declarations are overwritten correctly

TODO

figure out how to run git in CI
normalize EOLs for Windows CI
CliError with error message and additional info messages
Optional step for updating lockfile
Update release notes

changeset-bot · 2022-11-11T04:50:10Z

🦋 Changeset detected

Latest commit: fd5742f

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 16 packages

Name	Type
remix	Patch
@remix-run/dev	Patch
create-remix	Patch
@remix-run/architect	Patch
@remix-run/cloudflare	Patch
@remix-run/cloudflare-pages	Patch
@remix-run/cloudflare-workers	Patch
@remix-run/deno	Patch
@remix-run/eslint-config	Patch
@remix-run/express	Patch
@remix-run/netlify	Patch
@remix-run/node	Patch
@remix-run/react	Patch
@remix-run/serve	Patch
@remix-run/server-runtime	Patch
@remix-run/vercel	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

conversion to javascript is not a "migration" in that it does not help the user to upgrade to a newer version of Remix

MichaelDeBoey

WOW massive speed improvement! 😱
Also nice to now have a streamlined way of testing the CLI 😍

packages/remix-dev/__tests__/cli-test.ts

packages/remix-dev/__tests__/codemod-replaceRemixMagicImports-test.ts

packages/remix-dev/__tests__/fixtures/replace-remix-magic-imports/package.json

MichaelDeBoey · 2022-11-14T16:19:33Z

packages/remix-dev/__tests__/fixtures/replace-remix-magic-imports/tsconfig.json

@@ -19,6 +19,8 @@
    "skipLibCheck": true,

    // Remix takes care of building everything in `remix build`.
-    "noEmit": true
+    "noEmit": true,
+    "allowJs": true,


Maybe a good idea to keep this file in line with our templates?

Remix automatically adds these lines to the tsconfig if they are missing, which adds noise to the console for tests. So just decied to include it here since we enforce it anyway.

Yeah I know Remix automatically adds these.
That's why our templates have these values in tsconfig.json by default.
What I was suggesting was: copy over the tsconfig.json from one of our templates, so it's easier to auto-update (find/replace) if ever needed

Ah I see! I do like the isolation of fixtures being self-contained, but also like your suggestion as well. Let me think about it.

I didn't mean to copy over the file automatically, just copying over the content manually

So if we ever have to add extra defaults, we can just do all files automatically & we don't have to think about this file being different

MichaelDeBoey · 2022-11-14T17:28:45Z

packages/remix-dev/codemod/utils/git.ts

+    let gitStatus = await execa("git", ["status", "--porcelain"], {
+      cwd: dir,
+      encoding: "utf8",
+      maxBuffer: TEN_MEBIBYTE,
+    });


Was wondering if there's a special reason you switched from child_process to execa
It would be one less dependency to use.

Suggested change

let gitStatus = await execa("git", ["status", "--porcelain"], {

cwd: dir,

encoding: "utf8",

maxBuffer: TEN_MEBIBYTE,

});

let gitStatus = execFileSync("git", ["status", "--porcelain"], {

cwd: dir,

encoding: "utf8",

maxBuffer: TEN_MEBIBYTE,

})?.trim();

Good point. I do think that execa has a nicer API (autotrims output, provides more information about errors) but the main reason was due to it purported improvements for Windows support.

We were considering bundling all the CLI dependencies anyway so that running npx @remix-run/dev or npx create-remix wouldn't have to subsequently install a bunch of things. So I'll discuss this with the team to see if we think the benefits of execa are worth the added dependency.

packages/remix-dev/codemod/replace-remix-magic-imports/transform.ts

Compared to the previous jscodeshift-based migration: - codemod no longer depends on a network connection - babel's visitor API for traversing the AST is simpler - not spinning up workers for applying code transforms This ends up speeding up the codemod by ~10x and (hopefully 🤞) fixes some of the issues we were seeing in CI on Windows (since we think problems are mostly timeouts caused by slow tests or overhead for workers).

this fixture was incorrectly included as part of dependency upgrades: - #3917 - #3929 it should not have been updated since it is supposed to represent a Remix 1.3.x codebase

…ce-remix-magic-imports`

some tests can make use of the built javascript artifacts (e.g. `cli.js`) to run 8-10x faster than running with source Typescript (e.g. `cli.ts`)

Windows sometimes throws `EBUSY: resource busy or locked, rmdir` errors when attempting to removing the temporary directory. Retrying a couple times seems to get it to succeed. See https://github.com/jprichardson/node-fs-extra/issues?q=EBUSY%3A+resource+busy+or+locked%2C+rmdir

github-actions · 2022-11-16T07:07:03Z

🤖 Hello there,

We just published version v0.0.0-nightly-47b5b54-20221116 which includes this pull request. If you'd like to take it for a test run please try it out and let us know what you think!

Thanks!

…`-based codemods (#4572) * refactor(dev): extract `useColor` usages into `safe` utility * refactor(dev): remove outdated reference to compiler shims * fix(dev): remove access to `convert-to-javascript` migration via CLI conversion to javascript is not a "migration" in that it does not help the user to upgrade to a newer version of Remix * refactor(dev): rewrite `replace-remix-magic-exports` as a babel codemod Compared to the previous jscodeshift-based migration: - codemod no longer depends on a network connection - babel's visitor API for traversing the AST is simpler - not spinning up workers for applying code transforms This ends up speeding up the codemod by ~10x and (hopefully 🤞) fixes some of the issues we were seeing in CI on Windows (since we think problems are mostly timeouts caused by slow tests or overhead for workers). * test(dev): restore fixture this fixture was incorrectly included as part of dependency upgrades: - #3917 - #3929 it should not have been updated since it is supposed to represent a Remix 1.3.x codebase * test(dev): add tests for generic codemods and specifically for `replace-remix-magic-imports` * ci: run build before primary tests some tests can make use of the built javascript artifacts (e.g. `cli.js`) to run 8-10x faster than running with source Typescript (e.g. `cli.ts`) * test(dev): retry temp dir removal for windows ci Windows sometimes throws `EBUSY: resource busy or locked, rmdir` errors when attempting to removing the temporary directory. Retrying a couple times seems to get it to succeed. See https://github.com/jprichardson/node-fs-extra/issues?q=EBUSY%3A+resource+busy+or+locked%2C+rmdir * Create long-colts-remain.md

remix-cla-bot bot added the CLA Signed label Nov 11, 2022

pcattori changed the base branch from main to dev November 11, 2022 04:51

pcattori force-pushed the pedro/codemods branch 3 times, most recently from e1d3a9b to c994670 Compare November 12, 2022 21:27

pcattori changed the title ~~Pedro/codemods~~ refactor(dev): rewrite jscodeshift-based migrations as babel-based codemods Nov 14, 2022

pcattori force-pushed the pedro/codemods branch 2 times, most recently from 0fd531f to f4c3bb0 Compare November 14, 2022 03:43

pcattori added 3 commits November 13, 2022 22:44

refactor(dev): extract useColor usages into safe utility

7bf9425

refactor(dev): remove outdated reference to compiler shims

a6b800d

fix(dev): remove access to convert-to-javascript migration via CLI

474e80f

conversion to javascript is not a "migration" in that it does not help the user to upgrade to a newer version of Remix

pcattori force-pushed the pedro/codemods branch from f4c3bb0 to 9e85517 Compare November 14, 2022 03:44

pcattori marked this pull request as ready for review November 14, 2022 13:25

pcattori force-pushed the pedro/codemods branch 4 times, most recently from c9d75bc to 87c9473 Compare November 14, 2022 13:56

pcattori requested review from mcansh and MichaelDeBoey November 14, 2022 14:34

pcattori force-pushed the pedro/codemods branch from 87c9473 to 499ee93 Compare November 14, 2022 15:04

MichaelDeBoey changed the title ~~refactor(dev): rewrite jscodeshift-based migrations as babel-based codemods~~ refactor(remix-dev): rewrite jscodeshift-based migrations as babel-based codemods Nov 14, 2022

MichaelDeBoey added the package:dev label Nov 14, 2022

MichaelDeBoey reviewed Nov 14, 2022

View reviewed changes

pcattori force-pushed the pedro/codemods branch 4 times, most recently from a6aeafb to 2001d94 Compare November 15, 2022 02:29

pcattori added 2 commits November 14, 2022 22:38

test(dev): restore fixture

0e2c0b6

this fixture was incorrectly included as part of dependency upgrades: - #3917 - #3929 it should not have been updated since it is supposed to represent a Remix 1.3.x codebase

pcattori added 3 commits November 14, 2022 22:38

test(dev): add tests for generic codemods and specifically for `repla…

a0e0dca

…ce-remix-magic-imports`

ci: run build before primary tests

a6ce326

some tests can make use of the built javascript artifacts (e.g. `cli.js`) to run 8-10x faster than running with source Typescript (e.g. `cli.ts`)

pcattori force-pushed the pedro/codemods branch from 2001d94 to e58d1d3 Compare November 15, 2022 03:38

Create long-colts-remain.md

fd5742f

pcattori merged commit 70784af into dev Nov 15, 2022

pcattori deleted the pedro/codemods branch November 15, 2022 18:54

github-actions bot added the awaiting release This issue has been fixed and will be released soon label Nov 15, 2022

MichaelDeBoey mentioned this pull request Nov 16, 2022

test(remix-dev): bring replace-remix-magic-imports fixture in line with template #4619

Merged

This was referenced Nov 19, 2022

chore: Update version for release #4647

Closed

chore: Update version for release #4672

Closed

machour mentioned this pull request Dec 16, 2022

npx create-remix@latest error #4854

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(remix-dev): rewrite `jscodeshift`-based migrations as `babel`-based codemods #4572

refactor(remix-dev): rewrite `jscodeshift`-based migrations as `babel`-based codemods #4572

pcattori commented Nov 11, 2022 •

edited

Loading

changeset-bot bot commented Nov 11, 2022 •

edited

Loading

MichaelDeBoey left a comment

MichaelDeBoey Nov 14, 2022

pcattori Nov 14, 2022

MichaelDeBoey Nov 14, 2022

pcattori Nov 14, 2022

MichaelDeBoey Nov 14, 2022

MichaelDeBoey Nov 14, 2022

pcattori Nov 14, 2022

github-actions bot commented Nov 16, 2022

refactor(remix-dev): rewrite jscodeshift-based migrations as babel-based codemods #4572

refactor(remix-dev): rewrite jscodeshift-based migrations as babel-based codemods #4572

Conversation

pcattori commented Nov 11, 2022 • edited Loading

Summary

Results

BEFORE

AFTER

Testing strategy

TODO

changeset-bot bot commented Nov 11, 2022 • edited Loading

🦋 Changeset detected

MichaelDeBoey left a comment

Choose a reason for hiding this comment

MichaelDeBoey Nov 14, 2022

Choose a reason for hiding this comment

pcattori Nov 14, 2022

Choose a reason for hiding this comment

MichaelDeBoey Nov 14, 2022

Choose a reason for hiding this comment

pcattori Nov 14, 2022

Choose a reason for hiding this comment

MichaelDeBoey Nov 14, 2022

Choose a reason for hiding this comment

MichaelDeBoey Nov 14, 2022

Choose a reason for hiding this comment

pcattori Nov 14, 2022

Choose a reason for hiding this comment

github-actions bot commented Nov 16, 2022

refactor(remix-dev): rewrite `jscodeshift`-based migrations as `babel`-based codemods #4572

refactor(remix-dev): rewrite `jscodeshift`-based migrations as `babel`-based codemods #4572

pcattori commented Nov 11, 2022 •

edited

Loading

changeset-bot bot commented Nov 11, 2022 •

edited

Loading