fix(remix-dev/cli/migrate): fix `convert-to-javascript` migration #3987

MichaelDeBoey · 2022-08-12T21:33:39Z

This will partially resolve remix-run/indie-stack#134

changeset-bot · 2022-08-12T21:33:41Z

⚠️ No Changeset found

Latest commit: 11b2012

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

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

MichaelDeBoey · 2022-08-12T21:35:54Z

...ev/cli/migrate/migrations/convert-to-javascript/transform/createExportExpressionStatement.ts

+  exportDefaultDeclaration: ExportDefaultDeclaration
+) => {
+  /**
+   * HACK: Can't use casts nor type guards in a `jscodeshift` transform


We need to check if exportDefaultDeclaration.declaration isn't of type DeclarationKind (union), but there's unfortunately no other way than checking each possible union value separately, since we can't cast in a jscodeshift transform.

We also can't extract this into a separate method, as than we would need a type guard, which is also not possible in a jscodeshift transform.

Is this just to make TypeScript happy with this code or is it actually needed at runtime?

The whole hack is to make TS (actually jscodeshift) happy 😢

Because I think the following is valid 🤔

// ClassDeclaration export default class Foo {}; // FunctionDeclaration export default function foo(){}; // => module.exports = class Foo {}; module.exports = function foo(){}

Found a way to convert these as well

MichaelDeBoey · 2022-08-12T21:38:53Z

...li/migrate/migrations/convert-to-javascript/transform/createVariableDeclarationIdentifier.ts

+      isDefaultImport
+        ? j.memberExpression(callExpression, j.identifier("default"))
+        : callExpression


import Foo from 'foo'; import * as Bar from 'bar'; // => const Foo = require('foo').default; const Bar = require('bar');

This won't always be correct 😬 I think we've made a big mistake by building this into our CLI. There are too many edge cases 😬

This won't always be correct 😬

Can you give some examples where this won't be correct?
I'll try to add them.

I think we've made a big mistake by building this into our CLI.

We wanted to support JS & TS & (as you know) having this migration was the cleanest way of converting all files in a template/stack to JS.
Otherwise each stack had to do the conversion on its own.

I think the decision for doing it in our CLI was a good one, but it's a pain to think about every possible thing. 🙈

There are too many edge cases 😬

If only there was a package/library that could convert to ES5 without making the file a mess 🤔😅

I do think that the TS->JS conversion is better suited as a standalone tool, in its own repo, provided as-is without covering all edge cases, rather than being included in our migrations in our CLI. I've mentioned this before and feel even more strongly about it now.

I think "migrations" concept in our CLI should be reserved for helping automate upgrading through breaking changes.

That said, we currently offer the --no-typescript option during create... so we'd have to deprecate / remove that option to get out of the business of doing the TS->JS conversion. Which I'd be in favor of, but its a breaking change for the CLI.

We supported both JS & TS version of our templates/stacks from v1.0.0, so that's why I added the new convert-to-javascript migration.

Now that we come across these extra changes that need to be made, I can see a separate package working for stacks.
We would still have the "problem" that our templates will need to use it as well.

We could definitely stop officially supporting JS, but that would indeed be a breaking change, so needs to wait for v2.

If we want, we can already create a separate package that's solely responsible for the JS conversion (or we could put it in a scripts package if that's preferred) & let the CLI use this new package.

I do like the idea of moving this all to a separate package so it can be quickly iterated upon and remix can depend on that package until v2 when it can be removed.

@kentcdodds Do you mean a separate Remix package or like a separate personal package?

Probably a separate Remix package I think. But anyway, I think we should probably just merge this and move forward.

kentcdodds

Thanks for your work on this 👍

kentcdodds · 2022-08-15T15:57:24Z

Forgot to add a changeset. Added here: 3696554

chaance · 2022-08-16T16:12:12Z

@MichaelDeBoey Looks like this PR failed our checks on Windows, so we probably should have held off on merging. @mcansh is looking into it, but @MichaelDeBoey if you have any instincts as to why (since you wrote the code), let me know!

MichaelDeBoey · 2022-08-16T16:20:31Z

@chaance @mcansh The only thing I can think of is that it's just taking too long on Windows, since we now do extra transforms 🤔

So increasing the timeout would help I guess.

mcansh · 2022-08-16T16:26:17Z

it doesn't appear to be a timeout, it's set to 20s but we're failing at around 9.5s

this is the stack trace:

 ERR C:/Users/logan/AppData/Local/Temp/remix-tests-73raps3edq8/template-to-js/app/root.tsx Transformation error ()
Error:
    at Function.from (C:\Users\logan\Developer\remix\node_modules\ast-types\lib\types.js:504:27)
    at createExportExpressionStatementFromExportDefaultDeclaration (C:\Users\logan\Developer\remix\packages\remix-dev\cli\migrate\migrations\convert-to-javascript\transform\/createExportExpressionStatementFromExportDefaultDeclaration.ts:71:30)
    at NodePath.<anonymous> (C:\Users\logan\Developer\remix\packages\remix-dev\cli\migrate\migrations\convert-to-javascript\transform\/index.ts:67:7)
    at C:\Users\logan\Developer\remix\node_modules\jscodeshift\src\Collection.js:75:36
    at Array.forEach (<anonymous>)
    at Collection.forEach (C:\Users\logan\Developer\remix\node_modules\jscodeshift\src\Collection.js:74:18)
    at transform (C:\Users\logan\Developer\remix\packages\remix-dev\cli\migrate\migrations\convert-to-javascript\transform\/index.ts:64:32)
 ERR C:/Users/logan/AppData/Local/Temp/remix-tests-73raps3edq8/template-to-js/app/entry.server.tsx Transformation error ()
Error:
    at Function.from (C:\Users\logan\Developer\remix\node_modules\ast-types\lib\types.js:504:27)
    at createExportExpressionStatementFromExportDefaultDeclaration (C:\Users\logan\Developer\remix\packages\remix-dev\cli\migrate\migrations\convert-to-javascript\transform\/createExportExpressionStatementFromExportDefaultDeclaration.ts:71:30)
    at NodePath.<anonymous> (C:\Users\logan\Developer\remix\packages\remix-dev\cli\migrate\migrations\convert-to-javascript\transform\/index.ts:67:7)
    at C:\Users\logan\Developer\remix\node_modules\jscodeshift\src\Collection.js:75:36
    at Array.forEach (<anonymous>)
    at Collection.forEach (C:\Users\logan\Developer\remix\node_modules\jscodeshift\src\Collection.js:74:18)
    at transform (C:\Users\logan\Developer\remix\packages\remix-dev\cli\migrate\migrations\convert-to-javascript\transform\/index.ts:64:32)

MichaelDeBoey · 2022-08-16T16:36:53Z

Then it's strange, as I don't do anything that can't be done on Windows I think 🤔
It's just JavaScript + jscodeshift usage

The error seems to be about creating a FunctionExpression from a FunctionDeclaration.
I would assume that would just work 🤔

      ? j.functionExpression.from(exportDefaultDeclaration.declaration)

…tion (#3987)" This reverts commit 81bec18.

…tion (#3987)" (#4010)

mcansh · 2022-08-17T16:23:52Z

Then it's strange, as I don't do anything that can't be done on Windows I think 🤔 It's just JavaScript + jscodeshift usage

The error seems to be about creating a FunctionExpression from a FunctionDeclaration. I would assume that would just work 🤔
      ? j.functionExpression.from(exportDefaultDeclaration.declaration)

yeah it's weird, macOS has no troubles with it, but ast-types throws on Windows when comparing it's internal type as it has the "built" type as FunctionDeclaration, but it's actual type is still FunctionExpression, and to top it off with the same log in place on macOS doesn't log anything 👻

MichaelDeBoey · 2022-08-20T12:23:01Z

@mcansh That's really weird 🤔😅

I'll resubmit this PR with this FunctionDeclaration conversion excluded.
We can always add it again once it's fixed on ast-types' side (which I doubt since it's unmaintained for a long time already) or jscodeshift (which is more possible, since @Daniel15 @ElonVolo & @trivikr are working on bringing the package up to date)

MichaelDeBoey added the package:dev label Aug 12, 2022

MichaelDeBoey requested review from pcattori, kentcdodds and mcansh August 12, 2022 21:33

remix-cla-bot bot added the CLA Signed label Aug 12, 2022

MichaelDeBoey commented Aug 12, 2022

View reviewed changes

MichaelDeBoey mentioned this pull request Aug 12, 2022

fix: fix remix.init remix-run/indie-stack#138

Merged

MichaelDeBoey force-pushed the fix-convert-to-javascript-migration branch 4 times, most recently from 3c723de to 699279f Compare August 14, 2022 15:47

fix(remix-dev/cli/migrate): fix convert-to-javascript migration

11b2012

MichaelDeBoey force-pushed the fix-convert-to-javascript-migration branch from 699279f to 11b2012 Compare August 14, 2022 15:55

kentcdodds approved these changes Aug 15, 2022

View reviewed changes

kentcdodds merged commit 81bec18 into remix-run:dev Aug 15, 2022

MichaelDeBoey deleted the fix-convert-to-javascript-migration branch August 15, 2022 15:11

MichaelDeBoey added the awaiting release This issue has been fixed and will be released soon label Aug 15, 2022

mcansh added a commit that referenced this pull request Aug 17, 2022

Revert "fix(remix-dev/cli/migrate): fix convert-to-javascript migra…

2b6f63a

…tion (#3987)" This reverts commit 81bec18.

mcansh mentioned this pull request Aug 17, 2022

Revert "fix(remix-dev/cli/migrate): fix convert-to-javascript migration" #4010

Merged

kentcdodds pushed a commit that referenced this pull request Aug 17, 2022

Revert "fix(remix-dev/cli/migrate): fix convert-to-javascript migra…

f2db5f1

…tion (#3987)" (#4010)

This was referenced Aug 20, 2022

fix(remix-dev/cli/migrate): process default exports in convert-to-javascript migration #4030

Merged

j.functionExpression.from(functionDeclaration) is failing on Windows facebook/jscodeshift#520

Open

MichaelDeBoey removed the awaiting release This issue has been fixed and will be released soon label Aug 20, 2022

This was referenced Aug 20, 2022

b.functionExpression.from(functionDeclaration) is failing on Windows benjamn/ast-types#814

Open

b.functionExpression.from(functionDeclaration) is failing on Windows benjamn/recast#1170

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(remix-dev/cli/migrate): fix `convert-to-javascript` migration #3987

fix(remix-dev/cli/migrate): fix `convert-to-javascript` migration #3987

MichaelDeBoey commented Aug 12, 2022

changeset-bot bot commented Aug 12, 2022 •

edited

Loading

MichaelDeBoey Aug 12, 2022 •

edited

Loading

kentcdodds Aug 12, 2022

MichaelDeBoey Aug 12, 2022 •

edited

Loading

MichaelDeBoey Aug 13, 2022

MichaelDeBoey Aug 12, 2022

kentcdodds Aug 12, 2022

MichaelDeBoey Aug 12, 2022

pcattori Aug 13, 2022 •

edited

Loading

pcattori Aug 13, 2022 •

edited

Loading

MichaelDeBoey Aug 13, 2022

kentcdodds Aug 14, 2022

MichaelDeBoey Aug 14, 2022 •

edited

Loading

kentcdodds Aug 15, 2022

kentcdodds left a comment

kentcdodds commented Aug 15, 2022 •

edited

Loading

chaance commented Aug 16, 2022

MichaelDeBoey commented Aug 16, 2022

mcansh commented Aug 16, 2022 •

edited

Loading

MichaelDeBoey commented Aug 16, 2022 •

edited

Loading

mcansh commented Aug 17, 2022 •

edited

Loading

MichaelDeBoey commented Aug 20, 2022

fix(remix-dev/cli/migrate): fix convert-to-javascript migration #3987

fix(remix-dev/cli/migrate): fix convert-to-javascript migration #3987

Conversation

MichaelDeBoey commented Aug 12, 2022

changeset-bot bot commented Aug 12, 2022 • edited Loading

⚠️ No Changeset found

MichaelDeBoey Aug 12, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MichaelDeBoey Aug 12, 2022 • edited Loading

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

pcattori Aug 13, 2022 • edited Loading

Choose a reason for hiding this comment

pcattori Aug 13, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MichaelDeBoey Aug 14, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kentcdodds left a comment

Choose a reason for hiding this comment

kentcdodds commented Aug 15, 2022 • edited Loading

chaance commented Aug 16, 2022

MichaelDeBoey commented Aug 16, 2022

mcansh commented Aug 16, 2022 • edited Loading

MichaelDeBoey commented Aug 16, 2022 • edited Loading

mcansh commented Aug 17, 2022 • edited Loading

MichaelDeBoey commented Aug 20, 2022

fix(remix-dev/cli/migrate): fix `convert-to-javascript` migration #3987

fix(remix-dev/cli/migrate): fix `convert-to-javascript` migration #3987

changeset-bot bot commented Aug 12, 2022 •

edited

Loading

MichaelDeBoey Aug 12, 2022 •

edited

Loading

MichaelDeBoey Aug 12, 2022 •

edited

Loading

pcattori Aug 13, 2022 •

edited

Loading

pcattori Aug 13, 2022 •

edited

Loading

MichaelDeBoey Aug 14, 2022 •

edited

Loading

kentcdodds commented Aug 15, 2022 •

edited

Loading

mcansh commented Aug 16, 2022 •

edited

Loading

MichaelDeBoey commented Aug 16, 2022 •

edited

Loading

mcansh commented Aug 17, 2022 •

edited

Loading