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

fix(remix): Use import() to get react-router-dom in Express wrapper. #5810

Merged
merged 1 commit into from Sep 23, 2022

Conversation

onurtemizkan
Copy link
Collaborator

Details copied from: #5796 which is reverted by #5802

loadModule utility which we use to acquire react-router-dom can return null when called from Express wrapper.

I suspect the main reason is that we are not monkey-patching Remix core here, like we are doing in the base implementation, the package acquisition is not guaranteed to run when (or where?) react-router-dom is available.

Tried using dynamic imports, and it worked well on https://github.com/getsentry/vanguard

Parameterized Route Transaction

Root Transaction

Redirected Route Transaction

@@ -40,6 +42,10 @@ function wrapExpressRequestHandler(
res: ExpressResponse,
next: ExpressNextFunction,
): Promise<void> {
if (!pkg) {
pkg = await import(`${cwd()}/node_modules/react-router-dom`);
Copy link
Member

Choose a reason for hiding this comment

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

Will is break if folks are bundling stuff (like in a serverless function)?

Can we just call import directly on react-router-dom?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Will is break if folks are bundling stuff (like in a serverless function)?

I think it depends on the packager used, but it seems there are some workarounds if such thing happens: https://www.serverless.com/plugins/serverless-jetpack#handling-dynamic-import-misses. It also applies to loadModule (using module.require) as far as I understand.

Can we just call import directly on react-router-dom?

Tried that and it's working, but while building @sentry/remix I got a warning from Rollup because, it actually tried to resolve the deps from sentry-javascript/node_modules on build time.

@AbhiPrasad AbhiPrasad enabled auto-merge (squash) September 23, 2022 18:34
@AbhiPrasad AbhiPrasad merged commit a9e78b7 into master Sep 23, 2022
@AbhiPrasad AbhiPrasad deleted the onur/remix-express-router-pkg-import branch September 23, 2022 18:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants