feat(remix-dev/cli): add DX improvements for `migrate` command & `replace-remix-imports` migration #2670

pcattori · 2022-04-06T19:33:17Z

Migration should guide the user through 3 steps:

Replacing the remix package with @remix-run/* packages.
Removing remix setup from the postinstall script
Replacing import {...} from "remix" with import {...} from "@remix-run/{some package}"

Try it!

git fetch origin # or whatever you call remix-run/remix
git checkout pedro/fix-migrate
yarn build
node ./build/node_modules/@remix-run/dev/cli.js migrate

Example

packages/remix-dev/cli/migrate/migrations/replace-remix-imports/getTransformOptions.ts

packages/remix-dev/cli/migrate/migrations/replace-remix-imports/index.ts

packages/remix-dev/cli/migrate/migrations/replace-remix-imports/getTransformOptions.ts

packages/remix-dev/cli/migrate/migrations/replace-remix-imports/index.ts

packages/remix-dev/cli/run.ts

packages/remix-dev/cli/migrate/migrations/replace-remix-imports/getTransformOptions.ts

packages/remix-dev/cli/migrate/migrations/replace-remix-imports/index.ts

packages/remix-dev/cli/migrate/migrations/replace-remix-imports/getTransformOptions.ts

ryanflorence · 2022-04-07T01:43:48Z

I ran it on a couple apps, works great!

pcattori · 2022-04-10T14:24:33Z

I added types for @npmcli/package-json in packages/remiv-dev/types/ and tsconfig.json is configured to include **/* from remix-dev/. VS Code finds these custom types just fine, but our build step calls tsc -b and that command cannot find our custom types.

Blocked on this^

MichaelDeBoey · 2022-04-10T17:07:18Z

I added types for @npmcli/package-json in packages/remiv-dev/types/ and tsconfig.json is configured to include **/* from remix-dev/. VS Code finds these custom types just fine, but our build step calls tsc -b and that command cannot find our custom types.

@pcattori I had the same problem when I was first using is-git-clean & added types for it.
Creating a PR in https://github.com/DefinitelyTyped/DefinitelyTyped & using @types/is-git-clean fixed it.

I created DefinitelyTyped/DefinitelyTyped#59806, so this PR isn't blocked anymore and we can investigate the root cause later.

MichaelDeBoey · 2022-04-10T17:52:32Z

@pcattori I created #2718 (this PR is the base branch) as that was easier to do instead of typing it all out as a review here 🙈

pcattori · 2022-04-11T13:56:46Z

@pcattori I created #2718 (this PR is the base branch) as that was easier to do instead of typing it all out as a review here 🙈

@MichaelDeBoey I've incorporated the feedback from your PR into this PR.

kentcdodds

Looking great! Just a few small things.

kentcdodds · 2022-04-11T19:03:29Z

packages/remix-dev/__tests__/fixtures/replace-remix-imports/.github/workflows/deploy.yml

@@ -0,0 +1,205 @@
+name: 🚀 Deploy


Could we remove unnecessary files from the fixtures? I worry that including them will cause confusion and issues in the future when people are doing a big find/replace sort of thing in the codebase and they come across files like this and spend extra time to make sure they make proper changes when it actually doesn't even matter.

So let's include only the files that can justify their existence in the fixture (which is to say that most of the files here should probably be deleted or, at the very least, drastically simplified).

Removed a ton of files. Let me know if you think there's still too much cruft left in here.

kentcdodds · 2022-04-11T19:08:53Z

packages/remix-dev/__tests__/replace-remix-imports-test.ts

+};
+
+describe("`replace-remix-imports` migration", () => {
+  it("runs successfully", async () => {


Let's add a couple assertions in here that look for the text from "remix" and ensures it no longer exists in the codebase.

And perhaps we could run the build script in the fixture after the migration to make sure imports can be resolved properly.

I added checks that from "remix" is purged from the app post-migration.

As for running build, adding npm install and npm run build to the test makes the test go from sub-second to more than 1 minute on my machine (M1 Max) and becomes network dependent. I think I prefer leaving out the build check and keeping this test lean. What do you think @kentcdodds ?

Yeah, definitely don't want it to be network dependent 👍 We could probably simplify the app further (remove all the deps and stuff and make remix reference the local remix), but that's more work than it's worth. This is enough 👍

packages/remix-dev/cli/migrate/migrations/replace-remix-imports/index.ts

MichaelDeBoey

I'm just wondering why we need baa2183 & 6e1bc21.

Since we're building the whole @remix-run/dev package, it shouldn't matter if jscodeshift supports it out of the box or not I think? 🤔

…ts in transformations by design, transformations have a default export for compatibility with jscodeshift. but we also want to share code from transforms, necessitating named exports.

…hem to do post-migration steps also, improved DX by clearly delineating separate migration sections

…s a dependency

…emix-imports` migration

…ce-remix-imports`

… adapter

… migration inputs

… `Runtime` arg

… blog tutorial example

once DefinitelyTyped/DefinitelyTyped#59806 is merged, we can install `@types/@npmcli/package-json` instead

…and wants it

…ix` imports

… a dev dependency

…ated", not "new"

…ansform because jscodeshift does not have full typescript support for transforms

… in transform see facebook/jscodeshift#467

kentcdodds

This is great! Lots here that should hopefully help in future code transformations we will inevitably need to make 👍 Thanks!

kentcdodds · 2022-04-12T16:00:46Z

packages/remix-dev/__tests__/replace-remix-imports-test.ts

+};
+
+describe("`replace-remix-imports` migration", () => {
+  it("runs successfully", async () => {


Yeah, definitely don't want it to be network dependent 👍 We could probably simplify the app further (remove all the deps and stuff and make remix reference the local remix), but that's more work than it's worth. This is enough 👍

remix-cla-bot bot added the CLA Signed label Apr 6, 2022

pcattori changed the base branch from main to dev April 6, 2022 19:35

pcattori changed the title ~~Pedro/fix migrate~~ DX improvements for migrate command and replace-remix-imports migration Apr 6, 2022

pcattori requested a review from MichaelDeBoey April 6, 2022 19:42

MichaelDeBoey changed the title ~~DX improvements for migrate command and replace-remix-imports migration~~ feat(remix-dev/cli): add DX improvements for migrate command & replace-remix-imports migration Apr 6, 2022

MichaelDeBoey added package:dev feat:dx Issues related to the developer experience labels Apr 6, 2022

pcattori commented Apr 6, 2022

View reviewed changes

packages/remix-dev/cli/migrate/migrations/replace-remix-imports/getTransformOptions.ts Outdated Show resolved Hide resolved

pcattori commented Apr 6, 2022

View reviewed changes

packages/remix-dev/cli/migrate/migrations/replace-remix-imports/index.ts Outdated Show resolved Hide resolved

MichaelDeBoey requested changes Apr 6, 2022

View reviewed changes

pcattori force-pushed the pedro/fix-migrate branch from 25560fb to 65067c9 Compare April 6, 2022 20:18

MichaelDeBoey requested changes Apr 6, 2022

View reviewed changes

packages/remix-dev/cli/migrate/migrations/replace-remix-imports/index.ts Outdated Show resolved Hide resolved

packages/remix-dev/cli/migrate/migrations/replace-remix-imports/index.ts Outdated Show resolved Hide resolved

MichaelDeBoey reviewed Apr 6, 2022

View reviewed changes

packages/remix-dev/cli/run.ts Outdated Show resolved Hide resolved

pcattori force-pushed the pedro/fix-migrate branch 2 times, most recently from 1829eb3 to 58267dc Compare April 6, 2022 21:53

MichaelDeBoey requested changes Apr 6, 2022

View reviewed changes

MichaelDeBoey requested changes Apr 7, 2022

View reviewed changes

packages/remix-dev/cli/migrate/migrations/replace-remix-imports/getTransformOptions.ts Outdated Show resolved Hide resolved

pcattori force-pushed the pedro/fix-migrate branch 2 times, most recently from 6d93edd to db33d4b Compare April 10, 2022 14:23

MichaelDeBoey mentioned this pull request Apr 10, 2022

feat(remix-dev/cli): add DX improvements for migrate command & replace-remix-imports migration #2718

Closed

pcattori requested a review from MichaelDeBoey April 11, 2022 16:42

MichaelDeBoey approved these changes Apr 11, 2022

View reviewed changes

kentcdodds requested changes Apr 11, 2022

View reviewed changes

pcattori requested a review from kentcdodds April 11, 2022 23:44

MichaelDeBoey approved these changes Apr 12, 2022

View reviewed changes

pcattori added 26 commits April 12, 2022 10:56

chore(cli/migrate): silence warnings about mixing default/named expor…

8080bb1

…ts in transformations by design, transformations have a default export for compatibility with jscodeshift. but we also want to share code from transforms, necessitating named exports.

feat(cli/migrate): do not modify user's package.json, instead ask t…

2bf5f20

…hem to do post-migration steps also, improved DX by clearly delineating separate migration sections

refactor(cli/migrate): check git status as soon as possible

45558e6

test(cli): add --debug flag for migrate command in tests

928d1af

refactor(cli/migrate): combine imports from same module

11bdc74

feat(cli/migrate): auto-detect node runtime if @remix-run/serve i…

87a6a47

…s a dependency

feat(cli/migrate): show npm commands for manual steps in `replace-r…

c57111a

…emix-imports` migration

feat(cli/migrate): handle remix setup without runtime arg in `repla…

6553122

…ce-remix-imports`

feat(cli/migrate): auto-detect server runtime from presence of server…

6a38e76

… adapter

feat(cli/migrate): automatically update package.json

9a04568

refactor(cli/migrate): convenience function for resolving general all…

c4d0481

… migration inputs

feat(cli/migrate): user-friendly cli output for replace-remix-imports

4372234

refactor(cli/migrate): remove unused import

5dc8c6a

refactor(cli/migrate): explicit type for adapterToRuntime converter

048f26e

refactor(cli/migrate): remove redundant check for migration id

b262977

refactor(cli/migrate): explicitly type detectedRuntime utility with…

659c6a3

… `Runtime` arg

test(cli/migrate): replace-remix-imports runs successfully on (old)…

738f414

… blog tutorial example

style: ignore test fixtures

6d8bcea

chore(dev): remove reference to non-existant @mdx-js/mdx types

d6a7181

refactor(cli/migrate): ignore missing types for @npmcli/package-json

053b8f8

once DefinitelyTyped/DefinitelyTyped#59806 is merged, we can install `@types/@npmcli/package-json` instead

fix(cli/migrate): only print colorful output if terminal supports it …

a2495ab

…and wants it

test(cli/migrate): check that replace-remix-imports remove all `rem…

a6b98d0

…ix` imports

fix(cli/migrate): replace-remix-imports ensures @remix-run/dev is…

96e5bfe

… a dev dependency

fix(cli/migrate): package.json already exists, so refer to it as "upd…

1c9051b

…ated", not "new"

refactor(cli/migrate): separate transform-related exports from the tr…

6a7a688

…ansform because jscodeshift does not have full typescript support for transforms

fix(cli/migrate): fix all jscodeshift non-supported typescript syntax…

6963bcb

… in transform see facebook/jscodeshift#467

pcattori force-pushed the pedro/fix-migrate branch from 213415d to 6963bcb Compare April 12, 2022 14:56

kentcdodds approved these changes Apr 12, 2022

View reviewed changes

kentcdodds merged commit adb5def into dev Apr 12, 2022

kentcdodds deleted the pedro/fix-migrate branch April 12, 2022 16:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(remix-dev/cli): add DX improvements for `migrate` command & `replace-remix-imports` migration #2670

feat(remix-dev/cli): add DX improvements for `migrate` command & `replace-remix-imports` migration #2670

pcattori commented Apr 6, 2022 •

edited

Loading

ryanflorence commented Apr 7, 2022

pcattori commented Apr 10, 2022 •

edited

Loading

MichaelDeBoey commented Apr 10, 2022

MichaelDeBoey commented Apr 10, 2022 •

edited

Loading

pcattori commented Apr 11, 2022

kentcdodds left a comment

kentcdodds Apr 11, 2022

pcattori Apr 11, 2022

kentcdodds Apr 11, 2022

kentcdodds Apr 11, 2022

pcattori Apr 11, 2022 •

edited

Loading

kentcdodds Apr 12, 2022

MichaelDeBoey left a comment

kentcdodds left a comment

kentcdodds Apr 12, 2022

feat(remix-dev/cli): add DX improvements for migrate command & replace-remix-imports migration #2670

feat(remix-dev/cli): add DX improvements for migrate command & replace-remix-imports migration #2670

Conversation

pcattori commented Apr 6, 2022 • edited Loading

Try it!

Example

ryanflorence commented Apr 7, 2022

pcattori commented Apr 10, 2022 • edited Loading

MichaelDeBoey commented Apr 10, 2022

MichaelDeBoey commented Apr 10, 2022 • edited Loading

pcattori commented Apr 11, 2022

kentcdodds left a comment

Choose a reason for hiding this comment

kentcdodds Apr 11, 2022

Choose a reason for hiding this comment

pcattori Apr 11, 2022

Choose a reason for hiding this comment

kentcdodds Apr 11, 2022

Choose a reason for hiding this comment

kentcdodds Apr 11, 2022

Choose a reason for hiding this comment

pcattori Apr 11, 2022 • edited Loading

Choose a reason for hiding this comment

kentcdodds Apr 12, 2022

Choose a reason for hiding this comment

MichaelDeBoey left a comment

Choose a reason for hiding this comment

kentcdodds left a comment

Choose a reason for hiding this comment

kentcdodds Apr 12, 2022

Choose a reason for hiding this comment

feat(remix-dev/cli): add DX improvements for `migrate` command & `replace-remix-imports` migration #2670

feat(remix-dev/cli): add DX improvements for `migrate` command & `replace-remix-imports` migration #2670

pcattori commented Apr 6, 2022 •

edited

Loading

pcattori commented Apr 10, 2022 •

edited

Loading

MichaelDeBoey commented Apr 10, 2022 •

edited

Loading

pcattori Apr 11, 2022 •

edited

Loading