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.

Sign up for GitHub

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

Jump to bottom

Remix doesn't split server code #6318

Closed
1 task done
jaschaio opened this issue May 5, 2023 · 8 comments
Closed
1 task done

Remix doesn't split server code #6318

jaschaio opened this issue May 5, 2023 · 8 comments
Labels
bug:unverified needs-response We need a response from the original author about this issue/PR package:dev

Comments

@jaschaio
Copy link

jaschaio commented May 5, 2023

What version of Remix are you using?

1.16.0

Are all your remix dependencies & dev-dependencies using the same version?

  • Yes

Steps to Reproduce

Repository: https://github.com/jaschaio/remix-monorepo

Manual steps to reproduce:

Create a new directory and enter it:

mkdir -p remix-monorepo/packages
cd remix-monorepo/packages

Run create-remix@latest:

npx create-remix@latest

Choose app, Just the basics, Remix App Server, TypeScript and Yes:

image

Create another subdirectory within the remix-monorepo/packages folder with two files:

  1. remix-monorepo/packages/package.json
  2. remix-monorepo/packages/User.ts
mkdir services && touch services/package.json && touch services/User.ts

Fill the remix-monorepo/packages/package.json:

{
  "name": "services",
  "version": "1.0.0",
  "description": "",
  "main": "index.js",
  "type": "module",
  "scripts": {
    "test": "echo \"Error: no test specified\" && exit 1"
  },
  "author": "",
  "license": "ISC"
}

Fill the remix-monorepo/packages/User.ts:

// Any import from crypto will fail, e.G. createHmac, createDecipheriv or createHash
import { randomBytes } from 'crypto';

// Just pseudo code to make a point
export const getToken = ( length: number ) => {

    return randomBytes( Math.ceil( length / 2 ) )
        .toString( 'hex' )
        .slice( 0, length );

};

Add the services module as a dependency to the remix project

cd app && npm i ../services

Edit the app/app/root.tsx file:

import { cssBundleHref } from "@remix-run/css-bundle";
import type { LinksFunction } from "@remix-run/node";
import {
  Links,
  LiveReload,
  Meta,
  Outlet,
  Scripts,
  ScrollRestoration,
} from "@remix-run/react";

+ import { getToken } from 'services/User';

export const links: LinksFunction = () => [
  ...(cssBundleHref ? [{ rel: "stylesheet", href: cssBundleHref }] : []),
];

+export const loader = () => {
+
+  const token = getToken();
+
+  return {
+    token,
+  };

};

export default function App() {

+  console.log( useLoaderData );

  return (
    <html lang="en">
      <head>
        <meta charSet="utf-8" />
        <meta name="viewport" content="width=device-width,initial-scale=1" />
        <Meta />
        <Links />
      </head>
      <body>
        <Outlet />
        <ScrollRestoration />
        <Scripts />
        <LiveReload />
      </body>
    </html>
  );
}

Run npm run dev:

image

Expected Behavior

Remix should strip server only code from client bundles and not throw any errors when trying to bundle node dependencies like crypto.

Actual Behavior

Remix throws errors:

image

I get it as well with other build in node modules:

image

Just a handful of things that throw errors for me:

import { randomBytes } from 'crypto';
import { writeFile } from 'node:fs';
import { Worker } from 'node:worker_threads';
import { PassThrough } from 'node:stream';
import { buffer } from 'node:stream/consumers';
@paul-vd
Copy link

paul-vd commented May 5, 2023

It might be related: #6302

@pcattori
Copy link
Contributor

pcattori commented May 5, 2023

Related to #6259

@joejemmely
Copy link

joejemmely commented May 5, 2023 •
edited
Loading

To get rid of the The path "service/User" is imported... warning, you'll want to update the main field in remix-monorepo/packages/services /package.json to be index.ts (which you'll need to define and re-export from User). Alternatively you can configure exports using the exports fields if you want to exports from the modules directly.

Also, since you don't have a build step, you'll probably want Remix to bundle your service package. To do that you can configure serverDependenciesToBundle in your app remix.config.js

When you define routes outside of your remix app package:

  • if don't have a build step for your external routes and use remix as the bundler, client/server pruning work seamlessly as long as you tell remix to bundle the code for these external packages
  • if you do have a build step for your external routes, you'll want to separate the client and server code because remix won't able to do it for code that's already bundled.

@jaschaio
Copy link
Author

jaschaio commented May 6, 2023

@joejemmely thanks, that get's indeed rid of the The path "services/User" is imported... warning but doesn't help with the ERROR.

Returning to @remix-run/[email protected] resolves npm run dev for me, but npm run build continues to fail with a similar error:

image

@joejemmely
Copy link

Not a perfect solution but if you re-export the library from a .server.ts file it seems to work, check this example -> https://github.com/joejemmely/remix-mono/tree/main/services/remix-service/app/routes/a

@paul-vd
Copy link

paul-vd commented May 10, 2023

Not a perfect solution but if you re-export the library from a .server.ts file it seems to work, check this example -> joejemmely/remix-mono@main/services/remix-service/app/routes/a

This is not a solution in the context of this issue (monorepo), and just used the expected behavior of server only code when importing from .server also see #6302

@bvkimball bvkimball mentioned this issue May 19, 2023
Closed
1 task
This was referenced Jun 6, 2023
Closed
Closed
@jacob-ebey jacob-ebey mentioned this issue Jun 8, 2023
Merged
@MichaelDeBoey
Copy link
Member

We just published version 1.17.1-pre.0 which includes #6562 (a possible fix). If you'd like to take it for a test run please try it out and let us know what you think!

@MichaelDeBoey MichaelDeBoey added the needs-response We need a response from the original author about this issue/PR label Jun 16, 2023
@github-actions
Copy link
Contributor

This issue has been automatically closed because we haven't received a response from the original author 🙈. This automation helps keep the issue tracker clean from issues that are unactionable. Please reach out if you have more information for us! 🙂

@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale Jun 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug:unverified needs-response We need a response from the original author about this issue/PR package:dev
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
6 participants
@pcattori @brophdawg11 @joejemmely @MichaelDeBoey @jaschaio @paul-vd