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

Use import instead of require to fix ESM Remix apps #29

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

TooTallNate
Copy link

@TooTallNate TooTallNate commented Mar 15, 2024

This is a (partial) fix for #28.

You might want to just use it as inspiration since this won't work for the .cjs build of this package. Or maybe it's time to drop the .cjs build, considering the Remix templates are ESM at this point.

I also didn't try the dev mode, but I'm sure the purging of the require cache doesn't work correctly with this change. Not sure how to do that when using import (nodejs/node#49442).

@itsMapleLeaf
Copy link
Owner

So I eventually want to move to using remix vite, but I am interested in a short term fix for this before that happens (since it's not trivial, lol)

I think the approach I'd want to take is something like this:

try {
	await import(...)
} catch (importError) {
	try {
		require(...)
	} catch (requireError) {
		console.error("Failed to load build.", { importError, requireError })
	}
}

If you can update this PR and ensure it works for both CJS and ESM projects, I'd be happy to merge this and release!

@giuseppeg
Copy link

giuseppeg commented Aug 22, 2024

@itsMapleLeaf not sure we need the try/catch thing. Dynamic imports are allowed in CJS right?

import.meta won't be available in CJS but you can't try/catch that. I suggest that we remove this piece:

const buildPath =
typeof serverBuildOption === "string"
? require.resolve(serverBuildOption)
: undefined

Nonetheless I will try to scaffold a CJS app and test this version with imports.

I also didn't try the dev mode, but I'm sure the purging of the require cache doesn't work correctly with this change. Not sure how to do that when using import (nodejs/node#49442).

What we need is a timestamp on the statements to invalidate the cache. With that we can remove the purgeRequireCache helper.

Eg.

await import(`${buildPath}?${Date.now()}`);

Happy to make those changes if you agree (I have this in my fork and it works).

@itsMapleLeaf
Copy link
Owner

@itsMapleLeaf not sure we need the try/catch thing. Dynamic imports are allowed in CJS right?

import.meta won't be available in CJS but you can't try/catch that. I suggest that we remove this piece:

const buildPath =
typeof serverBuildOption === "string"
? require.resolve(serverBuildOption)
: undefined

Nonetheless I will try to scaffold a CJS app and test this version with imports.

I also didn't try the dev mode, but I'm sure the purging of the require cache doesn't work correctly with this change. Not sure how to do that when using import (nodejs/node#49442).

What we need is a timestamp on the statements to invalidate the cache. With that we can remove the purgeRequireCache helper.

Eg.

await import(`${buildPath}?${Date.now()}`);

Happy to make those changes if you agree (I have this in my fork and it works).

@giuseppeg Sure, sounds good to me. I'm not concerned about the implementation details, just that both CJS and ESM projects work 👍

@giuseppeg giuseppeg mentioned this pull request Aug 28, 2024
@giuseppeg
Copy link

@itsMapleLeaf implemented all the changes in #33 I added a ESM test app too

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.

3 participants