Skip to content

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

Merged
AbhiPrasad merged 1 commit intomasterfrom
onur/remix-express-router-pkg-import
Sep 23, 2022
Merged

fix(remix): Use import() to get react-router-dom in Express wrapper.#5810
AbhiPrasad merged 1 commit intomasterfrom
onur/remix-express-router-pkg-import

Conversation

@onurtemizkan
Copy link
Copy Markdown
Contributor

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

next: ExpressNextFunction,
): Promise<void> {
if (!pkg) {
pkg = await import(`${cwd()}/node_modules/react-router-dom`);
Copy link
Copy Markdown
Contributor

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
Copy Markdown
Contributor 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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants