fix(remix): Rework dynamic imports of react-router-dom.

[onurtemizkan]

Fixes: #5860
Ref: #5810 (comment)

---

Added react-router as a peer dependency to flag it as external, so Rollup won't complain in sentry-javascript's build time.

Edit:
Defined react-router and react-router-dom as externals in Rollup configuration, without making them peer dependencies.

---

Added import by name as the first priority (which will work in monorepos) and relative imports as the second if the former fails. Logging error at the end if the latter fails too, without breaking the whole application.

This implementation is similar to the loadModule implementation below, only the priorities are swapped. (The other way works too)

sentry-javascript/packages/utils/src/node.ts

Lines 46 to 63 in 69dfc35

	export function loadModule<T>(moduleName: string): T | undefined {
	let mod: T | undefined;
	try {
	mod = dynamicRequire(module, moduleName);
	} catch (e) {
	// no-empty
	}
	try {
	const { cwd } = dynamicRequire(module, 'process');
	mod = dynamicRequire(module, `${cwd()}/node_modules/${moduleName}`) as T;
	} catch (e) {
	// no-empty
	}
	return mod;
	}

Which makes me wonder if we could implement this as a utility, and start using it as a fallback wherever dynamicRequire may not work? (like Deno?)

[timfish]

I think we use dynamicRequire/loadModule in place of require() when we don't want bundlers to pick up loading of an optional dependency.

Does import('react-router-dom') not run into the same issues as using require()? Wont bundlers pick up this import, complain about the missing dependency and users will be forced to add react-router-dom to their externals?

[onurtemizkan]

I think we use dynamicRequire/loadModule in place of require() when we don't want bundlers to pick up loading of an optional dependency.

Does import('react-router-dom') not run into the same issues as using require()? Wont bundlers pick up this import, complain about the missing dependency and users will be forced to add react-router-dom to their externals?

Yes, that's not like a replacement of loadModule itself.

That came up when I worked on #5796, where loadModule returned null, and the specific case was the wrapper running on server runtime, instead of server build-time when we use loadModule for monkey-patching the library.

So, the alternative that worked was either a dynamic require or import, which forced us to add the module to externals when used a named import (in that case it was require('react-router-dom') or await import('react-router-dom').

It actually worked fine without any external definitions when using dynamic relative paths:
require('${cwd()}/node_modules/react-router-dom') or
import('${cwd()}/node_modules/react-router-dom')

And, we went with import + relative path as Remix works on Node 14+ where import is available, and require might not be safe in some environments like Deno. (I haven't experimented require in Deno, but it looks like it will only be available with a polyfill?)

Then, that broke monorepo projects where our relative paths can be wrong. (#5860)
So, I had to add the named import as an alternative. (with the externals)

Not sure it would be much used if we made this a utility, though. But can be an ESM friendly fallback for loadModule in such cases (if there are others).

[onurtemizkan]

Alternatively, we can traverse the directory tree up to some levels (2 - 3?), if the module is not found. That could allow us to avoid dealing with named imports and externals. I'm not sure if that would cover every case, though.

[timfish]

My understanding is that adding libraries to externals only fixes the issue for our bundling and users who are bundling themselves might need to do the same to avoid errors?

Dynamic paths for import/require does give warnings with webpack but I guess webpack is not used for Remix which is a bonus!

[timfish]

Sorry, I clicked the wrong button and closed the issue and now CI is running again.

If remix/esbuild happily builds/bundles an app without react-router-dom available without errors or warnings, your changes here are likely good!

[onurtemizkan]

My understanding is that adding libraries to externals only fixes the issue for our bundling and users who are bundling themselves might need to do the same to avoid errors?

Dynamic paths for import/require does give warnings with webpack but I guess webpack is not used for Remix which is a bonus!

Yes, exactly. Remix uses ESBuild, but still, like webpack, it throws errors when such thing happens. But, the case in Remix is that react-router is always there in the project, as Remix depends on it.

So, thinking now, this can only be applicable for opt-in integrations or specific SDKs as @sentry/remix after all. If we apply something like this to an auto-instrumented database integration, it can throw errors with no reason.

[lobsterkatie]

Sorry for coming late to this overall problem - maybe you covered this in previous discussions elsewhere besides #5810 and #5796 that I didn't see - but I'm not sure I understand why we need a dynamic require at all here. If we're worried about react-router-dom being available, can we not just make it a dependency of our own? If we don’t put any restrictions on the version, it should just end up getting deduped and using whatever version is already there because of remix, no? (Kinda like a peer dependency, but guaranteed to be there.) And if someone isn't using the express bit, it should get treeshaken away, I'd think.

Maybe I'm missing something?

[onurtemizkan]

If I remember correctly (not sure at all, it's been a while and this can be my internal conversation with myself 😅), the main reason we avoided using remix and react-router version 6 as dependencies was their type syntax not compatible with our TS version. So it seemed convenient to just use dynamicRequire, with our (shallowly) vendored type definitions (and that was not a problem until we implemented the Express wrapper). But I can check and try out if I recall that correctly.

[AbhiPrasad]

the main reason we avoided using remix and react-router version 6 as dependencies was their type syntax not compatible with our TS version

Yes this is correct - we couldn't vendor the types in. In addition, it was quite a bunch of code to vendor in, so we figured it wasn't worth it.

[AbhiPrasad]

I think there might be a better way around this, but in the spirit of just being able to unblock a user, I think we should 🚢 this and we can re-evaluate in the future.