Fixes #7951: prevent remix.init errors when init script is an ES6 module, update docs to clarify init script requirements #8100
Conversation
|
|
Hi @leothorp, Welcome, and thank you for contributing to Remix! Before we consider your pull request, we ask that you sign our Contributor License Agreement (CLA). We require this only once. You may review the CLA and sign it by adding your name to contributors.yml. Once the CLA is signed, the If you have already signed the CLA and received this response in error, or if you have any questions, please contact us at [email protected]. Thanks! - The Remix team |
leothorp
changed the title
|
Thank you for signing the Contributor License Agreement. Let's get this merged! 🥳 |
packages/remix-dev/cli/commands.ts
Outdated
| if (typeof initFn !== "function" && initFn.default) { | ||
| initFn = initFn.default; | ||
| } | ||
| try { | ||
| await initFn({ packageManager, rootDirectory: projectDir }); | ||
| if (typeof initFn === "function") { |
There was a problem hiding this comment.
Covered by the "succeeds for a remix.init script that doesn't default export a function" test.
packages/remix-dev/cli/commands.ts
Outdated
| @@ -44,12 +44,15 @@ export async function init( | |||
| }); | |||
| } | |||
|
|
|||
| let initFn = require(initScript); | |||
| let initFn = await import(initScript); | |||
There was a problem hiding this comment.
Covered by the "succeeds for remix.init defined as an ES6 module" test.
| @@ -31,6 +32,31 @@ jest.mock("child_process", () => { | |||
| }; | |||
| }); | |||
|
|
|||
| // Copy over the current build of @remix-run/dev and execute remix init from the node_modules dir there. | |||
| // Certain bugs only manifest when using this approach. | |||
| const execRemixInitInProject = (projectDir: string) => { | |||
There was a problem hiding this comment.
This stuff with copying over the newly-built version of @remix-run/dev and running it as a node module in the temp project is clunky, but was the only way I could reproduce the failing behavior of an es6 module init script throwing an error in the unit tests. With run(["init"]) the test always succeeds with the old code, even though remix init errors in actual use.
It seems like some aspect of the run(["init"]) calls used by other tests here doesn't fully replicate the real-world behavior of running remix init as a project dependency, so this was the only workaround I found initially .
leothorp
changed the title
…format-restrictions
…/github.com/leothorp/remix into relax-remix-init-module-format-restrictions
…/github.com/leothorp/remix into relax-remix-init-module-format-restrictions
|
@leothorp Sorry for the delay. I'm looking to get this in but wanted to first check whether there was a reason you closed it? |
@markdalgleish ah, sorry about the confusion- was just cleaning up my open PRs that I wasn't sure were still relevant, but happy to reopen. |
|
@markdalgleish anything else I can do to help get this over the line? no rush of course- and my apologies again for any confusion from closing it earlier. |
|
@markdalgleish is this still relevant? |
Closes: #7951
This addresses the two issues below, which are underlying #7951.
requireto import the init script, remix.init/index.js currently cannot be defined as an ESM module without throwingError [ERR_REQUIRE_ESM]: require() of ES Module. This file will be considered an ESM module by default if usingnpx create-remix@latest, creating a situation where defining a commonjs package.json in remix.init was required for it to not throw- despite the docs indicating that that the package.json should be optional.requirefor CJS modules) and adding a test covering ESM remix.init scripts.Testing Strategy: