fix(remix-dev/vite): preserve named exports when emptifying server/client only files

[hi-ogawa]

Closes: #7924, #7972

Based on the idea mentioned in #7924 (comment), I updated "remix-empty-server-modules" and "remix-empty-client-modules" to preserve named exports.
(It's worth noting that this won't probably fix #7957 since that issue is more related to "remix-remove-server-exports" plugin.)

I'm not sure if this approach is foolproof, but I thought it would be better to put together a PR to convey the idea clearly.
I would appreciate any feedback. Thanks!

note 1

One notable (and maybe critical) difference from simple export default {} is that, this will now potentially expose the export names from server only files in client assets if rollup didn't tree shake named exports.
I was thinking to keep using export default {} for build case, but it turns out rollup checks export names and throws an following error, so this approach might not be robust:

rollup error

4: import serverLibDefault, { serverLibNamed } from "../server-lib.server";
...
error during build:
RollupError: "serverLibNamed" is not exported by "app/server-lib.server.tsx", imported by "app/routes/only.tsx".

As one more important factor to decide on this, I think it's worth noting that vite build has minify: true by default, so probably for most practical purposes, we can assume that original names won't be visible directly on client.
Here are the examples of client assets output with corresponding minfiy flags:

minify: true (default)

// in build/build/assets/empty-server-client-only-a8a8a349.js
const a={};
var l={};
const o="clientLibDefault",
c=o,
u="clientLibNamed";
...

minify: false

const serverLibDefault = {};
var serverLibNamed = {};
const clientLibDefault = "clientLibDefault";
const clientLibDefault$1 = clientLibDefault;
const clientLibNamed = "clientLibNamed";
...

note 2

I also noticed that pre-vite remix applies "empifying" transform only for user application code, but vite version currently applies to all files including node_modules.

remix/packages/remix-dev/compiler/plugins/emptyModules.ts

Lines 21 to 26 in eb2f7cb

	// Limit this behavior to modules found in only the `app` directory.
	// This allows node_modules to use the `.server.js` and `.client.js`
	// naming conventions with different semantics.
	path
	.resolve(args.resolveDir, args.path)
	.startsWith(config.appDirectory)

I'm not sure if this change was intentional, but if it's still meant to be applied only for user code, then I think we could add additional check such as id.startsWith(cachedPluginConfig.appDirectory).

[changeset-bot]

🦋 Changeset detected

Latest commit: eb2f7cb

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 16 packages

Name	Type
@remix-run/dev	Patch
create-remix	Patch
remix	Patch
@remix-run/architect	Patch
@remix-run/cloudflare	Patch
@remix-run/cloudflare-pages	Patch
@remix-run/cloudflare-workers	Patch
@remix-run/css-bundle	Patch
@remix-run/deno	Patch
@remix-run/eslint-config	Patch
@remix-run/express	Patch
@remix-run/node	Patch
@remix-run/react	Patch
@remix-run/serve	Patch
@remix-run/server-runtime	Patch
@remix-run/testing	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[changeset-bot]

⚠️ No Changeset found

Latest commit: aa14d75

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

[gustavopch]

Patching @remix-run/dev with these changes fixed most of the errors I was getting, but I still get this one:

app/monitoring.ts (38:18) "captureRemixServerException" is not exported by "node_modules/@sentry/remix/esm/index.client.js", imported by "app/monitoring.ts".

Context:

• I have this file app/monitoring.ts that exports some logging functions that are used both on the client and on the server.
• It contains import * as Sentry from '@sentry/remix' and it has a function that calls Sentry.captureRemixServerException(...) (only on the server though).
• Trying to wrap that call with if (import.meta.env.SSR) didn't make the error disappear.
• Using a dynamic import like import('@sentry/remix').then(Sentry => Sentry.captureRemixServerException(...)) made the error disappear.

[hi-ogawa]

@gustavopch Thanks for testing this patch! However, I'm suspecting the issue you are seeing with @sentry/remix might be due to a different cause.

I tested basic setup on current remix vite (so without this PR's patch) https://stackblitz.com/edit/remix-run-remix-ikymah?file=app%2Futils.ts.

First, I can also see vite build (client build) showing the same message, but this is only a warning by rollup to say client build doesn't include this export. So as long as this export is not used in client code, app should be still functional.

reveal screenshot

I think what's odd is that @sentry/remix is swapping out @sentry/remix code for server and client by:

https://github.com/getsentry/sentry-javascript/blob/ff416aeb41aa729c0ed02459e8010ae3fa4016f4/packages/remix/package.json#L16-L17

  "module": "build/esm/index.server.js",
  "browser": "build/esm/index.client.js",

which itself is fine and a common thing to do, but these two modules are "incompatible" in ESM sense since these two files exports different names (e.g. client one is missing captureRemixServerException named export).
I think what @sentry/remix could do is to export captureRemixServerException from index.client.js but make its implementation to throw something like new Error("not implemented for client");

[gustavopch]

@hi-ogawa Thanks for investigating and sharing the repro on StackBlitz. 🙏

I've opened an issue there: getsentry/sentry-javascript#9594.

[pcattori]

Superceded by #8200

Specifically, we want any .server imports that would end up in the client bundle to cause build time failures. However, code .client imports often needs to be conditionally run as determined by runtime checks.

---

@hi-ogawa thanks for your work on this! Adapted your code for preserving .client exports in #8200 and added you as a co-author in the commit.