Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: move to ESM #255

Merged
merged 1 commit into from
Oct 3, 2024
Merged

Conversation

MichaelDeBoey
Copy link
Member

No description provided.

@brophdawg11
Copy link
Contributor

I tested this out but ran into an issue with the init script - I guess we need to update that here too:
https://github.com/remix-run/indie-stack/blob/main/remix.init/index.js#L263

~/me/shopify/repos/playgrounds> npx create-remix@latest --template https://github.com/MichaelDeBoey/indie-stack/tree/move-to-ESM
? Where would you like to create your app? temp
? TypeScript or JavaScript? TypeScript
? Do you want me to run `npm install`? Yes
npm WARN deprecated @npmcli/[email protected]: This functionality has been moved to @npmcli/fs

added 1337 packages, and audited 1338 packages in 1m

295 packages are looking for funding
  run `npm fund` for details

2 moderate severity vulnerabilities

To address all issues (including breaking changes), run:
  npm audit fix --force

Run `npm audit` for details.
💿 Running remix.init script
🚨 Oops, remix.init failed

ENOENT: no such file or directory, lstat '/Users/matt/me/shopify/repos/playgrounds/temp/.eslintrc.repo.js'

@MichaelDeBoey
Copy link
Member Author

@brophdawg11 Forgot to rename the file to remove in remix.init 🙈
Should be fine now

@brophdawg11
Copy link
Contributor

It got farther this time!

All migrations have been successfully applied.
Environment variables loaded from .env
Running seed command `ts-node --require tsconfig-paths/register prisma/seed.ts` ...
TypeError [ERR_UNKNOWN_FILE_EXTENSION]: Unknown file extension ".ts" for /Users/matt/me/shopify/repos/playgrounds/temp/prisma/seed.ts
    at new NodeError (node:internal/errors:405:5)
    at Object.getFileProtocolModuleFormat [as file:] (node:internal/modules/esm/get_format:79:11)
    at defaultGetFormat (node:internal/modules/esm/get_format:124:36)
    at defaultLoad (node:internal/modules/esm/load:84:20)
    at nextLoad (node:internal/modules/esm/loader:163:28)
    at ESMLoader.load (node:internal/modules/esm/loader:603:26)
    at ESMLoader.moduleProvider (node:internal/modules/esm/loader:457:22)
    at new ModuleJob (node:internal/modules/esm/module_job:64:26)
    at ESMLoader.#createModuleJob (node:internal/modules/esm/loader:480:17)
    at ESMLoader.getModuleJob (node:internal/modules/esm/loader:434:34) {
  code: 'ERR_UNKNOWN_FILE_EXTENSION'
}

An error occurred while running the seed command:
Error: Command failed with exit code 1: ts-node --require tsconfig-paths/register prisma/seed.ts
🚨 Oops, remix.init failed

Command failed: npm run setup

I ran into this same error on a project of mine recently and I forget what I did to resolve it. Going to go peek at the commit history...

@brophdawg11
Copy link
Contributor

brophdawg11 commented Aug 31, 2023

Ahh - I added this to package.json:

  "prisma": {
    "seed": "ts-node-esm prisma/seed.ts"
  }

Which looks like it's just a shorthand for ts-node --esm

@MichaelDeBoey
Copy link
Member Author

MichaelDeBoey commented Sep 1, 2023

@brophdawg11 I've added the --esm flag to the seed script 👍
I also updated module in tsconfig.json to ES2020 as ts-node wasn't recognizing import statements when module is set to CommonJS

@MichaelDeBoey MichaelDeBoey force-pushed the move-to-ESM branch 2 times, most recently from 69ee48e to 757b062 Compare September 1, 2023 13:56
Copy link

@bcbrian bcbrian left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently to get the upgrade to v2 to work I need to change the devserver script in the package.json to look like

"dev:serve": "binode --require ./mocks -- @remix-run/serve:remix-serve ./build/index.js",

will these changes fix needing to make this change?

@brophdawg11
Copy link
Contributor

OK the install completes without issue now! I'm still seeing some ESM/CJC issues with the binode/msw setup though:

> dev:serve
> binode --require ./mocks -- @remix-run/serve:remix-serve ./build/index.js

node:internal/modules/cjs/loader:1307
      throw err;
      ^

[Error [ERR_REQUIRE_ESM]: require() of ES Module /Users/matt/me/shopify/repos/playgrounds/temp/mocks/index.js not supported.
index.js is treated as an ES module file as it is a .js file whose nearest parent package.json contains "type": "module" which declares all .js files in that package scope as ES modules.
Instead rename index.js to end in .cjs, change the requiring code to use dynamic import() which is available in all CommonJS modules, or change "type": "module" to "type": "commonjs" in /Users/matt/me/shopify/repos/playgrounds/temp/package.json to treat all .js files as CommonJS (using .mjs for all ES modules instead).
] {
  code: 'ERR_REQUIRE_ESM'
}

It looks like binode is CJS only at the moment to it can't pull in the ESM mocks file nor the ESM built file. So do we need to make binode ESM friendly or find another way to import msw mocks?

@pcattori
Copy link

pcattori commented Sep 5, 2023

You can use the built-in NODE_OPTIONS instead of binode to avoid CJS/ESM issues:

NODE_OPTIONS="--require ./mocks" remix-serve ./build/index.js

@MichaelDeBoey
Copy link
Member Author

MichaelDeBoey commented Sep 5, 2023

@pcattori I've updated this stack to use NODE_OPTIONS instead, but I still get the ESM error @brophdawg11 mentioned when running npm run dev/npm run dev:serve/npm run start:mocks

@MichaelDeBoey MichaelDeBoey force-pushed the move-to-ESM branch 2 times, most recently from 45a4d33 to 7dab4c6 Compare September 5, 2023 20:13
@mcansh
Copy link
Contributor

mcansh commented Sep 5, 2023

You can use the built-in NODE_OPTIONS instead of binode to avoid CJS/ESM issues:

NODE_OPTIONS="--require ./mocks" remix-serve ./build/index.js

gonna want to use NODE_OPTIONS=\"--import ./mocks/index.js\" as --require only works for cjs

https://nodejs.org/api/cli.html#--importmodule

@MichaelDeBoey
Copy link
Member Author

@mcansh --import was only introduced in Node v19 though and the stack currently supports v14
As we're almost releasing Remix v2, we could go for v18 already I think, but I'm not really in favor of setting the minimum Node version to anything higher tbh 🤔

Reverting the imports in mocks/index.js back to use require gives back the older ESM error though 😕

@brophdawg11 @pcattori WDYT?

@mcansh
Copy link
Contributor

mcansh commented Sep 6, 2023

Reverting the imports in mocks/index.js back to use require gives back the older ESM error though 😕

that would need to be mocks/index.cjs and the serverBuildPath would need to be "./build/index.cjs" until remix v2. although it appears remix (or esbuild) (just checked, esbuild ./src/index.ts --outfile=./dist/index.cjs does what is expected) won't write the file if the extension is cjs while it does if it's mjs

edit: condition needed here https://github.com/remix-run/remix/blob/main/packages/remix-dev/compiler/server/write.ts#L14 (PR: remix-run/remix#7348 that should be back ported to remix v1)

diff --git a/package.json b/package.json
index 1f6655f..74cacb4 100644
--- a/package.json
+++ b/package.json
@@ -6,14 +6,14 @@
   "scripts": {
     "build": "remix build",
     "dev": "remix dev -c \"npm run dev:serve\"",
-    "dev:serve": "NODE_OPTIONS=\"--require ./mocks\" remix-serve ./build/index.js",
+    "dev:serve": "NODE_OPTIONS=\"--require ./mocks/index.cjs\" remix-serve ./build/index.cjs",
     "format": "prettier --write .",
     "format:repo": "npm run format && npm run lint:repo -- --fix",
     "lint": "eslint --cache --cache-location ./node_modules/.cache/eslint .",
     "lint:repo": "npm run lint -- --config .eslintrc.repo.cjs",
     "setup": "prisma generate && prisma migrate deploy && prisma db seed",
     "start": "remix-serve build",
-    "start:mocks": "NODE_OPTIONS=\"--require ./mocks\" remix-serve ./build/index.js",
+    "start:mocks": "NODE_OPTIONS=\"--require ./mocks/index.cjs\" remix-serve ./build/index.cjs",
     "test": "vitest",
     "test:e2e:dev": "start-server-and-test dev http://localhost:3000 \"npx cypress open\"",
     "pretest:e2e:run": "npm run build",
diff --git a/remix.config.js b/remix.config.js
index feda42d..8ec75fe 100644
--- a/remix.config.js
+++ b/remix.config.js
@@ -11,6 +11,7 @@ export default {
   },
   ignoredRouteFiles: ["**/.*", "**/*.test.{js,jsx,ts,tsx}"],
   postcss: true,
-  serverModuleFormat: "esm",
+  serverModuleFormat: "cjs",
   tailwind: true,
+  serverBuildPath: "./build/index.cjs",
 };

@brophdawg11
Copy link
Contributor

I can't see the commit-by-commit changes due to the rebase/force-push but I'm back to getting this error on install so I can't even get to the NODE_OPTIONS stuff to test it out?

Do others get the same error?

npx create-remix@latest --template https://github.com/MichaelDeBoey/indie-stack/tree/move-to-ESM

@MichaelDeBoey
Copy link
Member Author

MichaelDeBoey commented Sep 6, 2023

@brophdawg11 I just ran the command you posted and the seed works fine on my end

dev:serve is still failing without @mcansh's remix-run/remix#7348 though

@brophdawg11
Copy link
Contributor

I left a comment in #257 but due to all the headaches with the ESM stuff I think we should leave the indie stack on CJS for the v2 launch and then convert it to ESM as a separate undertaking. The changes in the v2 branch contain a v2-compatible CJS stack

@brophdawg11
Copy link
Contributor

Huh @MichaelDeBoey What version of node are you on? I upgraded to 20.5.1 this AM to test some other stuff so that's what it was failing on for me

@MichaelDeBoey
Copy link
Member Author

@brophdawg11 I'm on Node v16

@brophdawg11
Copy link
Contributor

yeah, it seems node 20 is the culprit, I just downgraded to 18 and it worked :/

@brophdawg11
Copy link
Contributor

Re-posting from #257 but I think we should hold off on the ESM conversions since they've opened a can of worms. Moving stacks to ESM is not a prerequisite for releasing Remix v2 - they just need to specify serverModuleFormat: 'cjs', which they do. So I would vote we:

  • get Remix v2 out the door with the stack as CJS
  • Update the stacks for v2 stable (remove future flags, V2_ types, etc.)
  • Then do a pass to convert stacks to them to ESM against a stable v2

That way we're not finding and trying to backfill bugs we've already fixed in 2.x. Nor do we have to worry about compatibility on anything less than node 18.

If everyone's on board, maybe we move these back to draft for the time being?

@dvnrsn
Copy link

dvnrsn commented Nov 14, 2023

What's the status on this? I just spent a few hours attempting to move to ESM and it was enough of a pain I thought I'd start this effort if it wasn't done already. I am on remix-run 2.1. I don't fully understand your list there, Matt. It looks like step one is done. What are the future flags / v2 types?

@MichaelDeBoey MichaelDeBoey force-pushed the move-to-ESM branch 2 times, most recently from 66a6414 to 8110f1d Compare April 9, 2024 19:39
@MichaelDeBoey MichaelDeBoey marked this pull request as ready for review April 9, 2024 19:52
@MichaelDeBoey MichaelDeBoey force-pushed the move-to-ESM branch 3 times, most recently from d0fb4ed to 0442ed9 Compare September 10, 2024 22:44
@brophdawg11
Copy link
Contributor

v2 is stable and we're prepping for the next release which will be React Router 7 (see this post and this post).

I don't think there's anything holding this up from moving forward.

I ran npx create-remix --template https://github.com/MichaelDeBoey/indie-stack/tree/move-to-ESM and it failed on prisma db seed with the ts-node error which I think will be resolved by #298:

Running seed command `ts-node --esm -r tsconfig-paths/register prisma/seed.ts` ...
TypeError [ERR_UNKNOWN_FILE_EXTENSION]: Unknown file extension ".ts" for /.../temp/prisma/seed.ts

Once we merge that, let's give this another shot and if all's well we can get it merged.

@MichaelDeBoey
Copy link
Member Author

@brophdawg11 I just rebased this branch onto latest main and ran npx create-remix@latest --template https://github.com/MichaelDeBoey/indie-stack/tree/move-to-ESM, which ran without any error

Copy link
Contributor

@brophdawg11 brophdawg11 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thanks for seeing this through!

@MichaelDeBoey MichaelDeBoey merged commit 7ebeacc into remix-run:main Oct 3, 2024
6 checks passed
@MichaelDeBoey MichaelDeBoey deleted the move-to-ESM branch October 3, 2024 21:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants