QwikDev / qwik

Instant-loading web apps, without effort
https://qwik.dev
MIT License
20.48k stars 1.26k forks source link

[📖] How to avoid server only code to be shipped in the client bundle #5160

Open juanpmarin opened 10 months ago

juanpmarin commented 10 months ago

Suggestion

Hi! I recently detected that some code that should not be in my client bundle, is there. I tried to find some guidance on how to avoid that, remix, for example, has a very good document on the subject: https://remix.run/docs/en/1.19.3/guides/constraints#server-code-pruning

Is there something similar for qwik that I could not find? Is there something similar to .server.ts files in remix for qwik? What's the best way to analyze qwik's client bundle?

mhevery commented 10 months ago

That is a very generic question. Can you provide an example of what happened?

juanpmarin commented 10 months ago

Although it is a generic question, it would be great to have documentation on it, similar to the document that remix has.

In my specific case, this is what I found:

We don't use qwik to connect to our database directly, but instead we have services that we call using gRPC and https://connectrpc.com/

We're also using qwik server side apis like sharedMap to share some context from layouts to specific routes, modular-forms is also being used to handle our form logic.

I was checking the code generated in the dist/builder folder:

This is one of our formAction$:

export const usePostNewQuote = formAction$<RecipientForm>(
  async (recipient, { redirect, platform, sharedMap, request, env }) => {
    const draftsRepository = DraftsRepository.fromPlatform(platform);
    const tracker = DefaultAnalyticsTracker.fromEnv(env);

    const draft = sharedMap.get("draft") as Draft;
    const quote = sharedMap.get("quote") as Quote;
    const user = sharedMap.get("user") as User;

    const requiresDocument = checkRequiresDocument(draft.courier);

    if (requiresDocument && !recipient.document) {
      throw new FormError<RecipientForm>("Document is required", {
        document: "required",
      });
    }

    if (requiresDocument && !recipient.documentType) {
      throw new FormError<RecipientForm>("Document type is required", {
        documentType: "required",
      });
    }

    await draftsRepository.save({
      ...draft,
      recipient: {
        ...recipient,
        document: recipient.document || "",
        documentType: recipient.documentType || "",
      },
    });

    await tracker.track(
      "Shipping - Quote recipient set",
      {
        quote: quote.name,
      },
      {
        user: user.name,
        request,
      },
    );

    throw redirect(302, `../summary`);
  },
  zodForm$(RecipientSchema),
);

and I found that piece of code in the client bundle, in this path dist/build/q-78c6dbe1.js:

  W = async (
    e,
    { redirect: p, platform: n, sharedMap: o, request: r, env: d },
  ) => {
    const i = S.fromPlatform(n),
      t = g.fromEnv(d),
      s = o.get("draft"),
      a = o.get("quote"),
      c = o.get("user"),
      m = w(s.courier);
    throw m && !e.document
      ? new h("Document is required", { document: "required" })
      : m && !e.documentType
      ? new h("Document type is required", { documentType: "required" })
      : (await i.save({
          ...s,
          recipient: {
            ...e,
            document: e.document || "",
            documentType: e.documentType || "",
          },
        }),
        await t.track(
          "Shipping - Quote recipient set",
          { quote: a.name },
          { user: c.name, request: r },
        ),
        p(302, "../summary"));
  };

I also found blocks like this in the q-manifest.json, but grpc and connect are only being used server side:

image

gioboa commented 10 months ago

Can you create a minimal repo to investigate in this issue please? 🙏

juanpmarin commented 10 months ago

@gioboa sure! Here it is: https://github.com/juanpmarin/qwik-issues-repro

Repro steps:

In my case, I found the server code in dist/build/q-335eafed.js:

import { HelloSchema as t } from "./q-7498aa87.js";
import "./q-225dcdfb.js";
import "./q-a4f8d898.js";
import "./q-f55dd5f2.js";
const m = t,
  p = (o, { redirect: s }) => {
    throw (
      (console.log("VALUES", o),
      console.log("THIS IS SERVER ONLY CODE"),
      s(302, "/success"))
    );
  };
export { p as s_AA5LgweUUMM, m as s_yvfvwzPD6Bs };
gioboa commented 10 months ago

I will look at this. 👍

UPDATE

I replaced formAction$ from modular-forms with Qwik routeAction$ and there is no server code in the client side. This is an important thing to say because Qwik with its APIs is working correctly.

fabian-hiller commented 9 months ago

@gioboa what do you think needs to be changed to prevent this from happening? formAction$ builds on globalActionQrl. https://github.com/fabian-hiller/modular-forms/blob/v0.20.2-qwik/packages/qwik/src/actions/formAction%24.ts#L68

maiieul commented 9 months ago

@fabian-hiller might be because there is no /*#__PURE__*/ before implicit$FirstArg(formActionQrl)?

fabian-hiller commented 9 months ago

Yes, maybe that's the reason. Can @mhevery or @manucorporat confirm this? If so, I'll try to publish an update in the evening.

gioboa commented 9 months ago

I manually added /* @__PURE__ */ in the library but this doesn't solve the problem

Screenshot 2023-09-26 at 18 35 33
mhevery commented 9 months ago

So, the optimizer treats certain functions as special: https://github.com/BuilderIO/qwik/blob/7e85de911654ec03b911e128d9148931b2fcf661/packages/qwik/src/optimizer/src/plugins/plugin.ts#L36-L45C1

As of right now, that list is hard-coded. If I add formAction$ to the list, everything works as expected.

I think the solution here is to make that list configurable, and the question would be how to configure it:

  1. We could pass it as option to qwikVite() in vite.config.ts
  2. But I think a better solution would be for the plugin to somehow read the package.json of the third-party library and get that list from there.

But a bigger question is, does this cause problems? Even though it makes it to the client bundle, it should never be loaded. (Or is the issue that sometimes it is loaded as a side-effect?)

fabian-hiller commented 9 months ago

Yes, as an author I also prefer the second option with the package.json. I think some people want to hide the server-side implementation, and it also reduces network traffic if that code is not loaded at all.

juanpmarin commented 9 months ago

@mhevery I think it does cause problems, I see it as a security risk, details about internal infrastructure, database schema, secrets could be filtered and be used to attack

juanpmarin commented 9 months ago

@mhevery @gioboa what about a naming convention for the optimizer to find server side functions? Like functions ending in action$

mhevery commented 9 months ago

@mhevery I think it does cause problems, I see it as a security risk, details about internal infrastructure, database schema, secrets could be filtered and be used to attack

not sure I understand this...

mhevery commented 9 months ago

@mhevery @gioboa what about a naming convention for the optimizer to find server side functions? Like functions ending in action$

Yes, but then the developer will have to see the extra name like server.

mhevery commented 9 months ago

Any chance anyone could take this on and add the ability to load additional names from the package.json?

fabian-hiller commented 9 months ago

I could imagine it. However, I find little time at the moment and it could take a while. Therefore it might be better if someone else creates a PR with a first draft and I review it.

wmertens commented 4 months ago

But guarding code with if (isServer) will work no matter what, right?

adamayres commented 1 month ago

I recently ran into this problem when migrating some existing forms that used routeAction$ and globalAction$ to use formAction$ in the Modular Form lib.

But a bigger question is, does this cause problems? Even though it makes it to the client bundle, it should never be loaded. (Or is the issue that sometimes it is loaded as a side-effect?)

In my specific case I had two formAction$'s, one that registers a user and one that logins in a user that use Drizzle, Postgres, Lucia, and Oslo. When attempting to build I run into two classes of issues:

1.) Warnings from Vite that there are modules being included in a client bundle that have been externalized.

[plugin:vite:resolve] [plugin vite:resolve] Module "os" has been externalized for browser compatibility, imported by "/Users/adam.ayres/dev/buttaplz/node_modules/.pnpm/postgres@3.4.4/node_modules/postgres/src/index.js". See https://vitejs.dev/guide/troubleshooting.html#module-externalized-for-browser-compatibility for more details.

2.) An error during the build that fails the build. I assume in this case it is failing on browserifying the crypto module.

error during build:
RollupError: Unexpected character '�'
    at getRollupError (file:///Users/adam.ayres/dev/buttaplz/node_modules/.pnpm/rollup@4.13.0/node_modules/rollup/dist/es/shared/parseAst.js:376:41)
    at ParseError.initialise (file:///Users/adam.ayres/dev/buttaplz/node_modules/.pnpm/rollup@4.13.0/node_modules/rollup/dist/es/shared/node-entry.js:11158:28)
    at convertNode (file:///Users/adam.ayres/dev/buttaplz/node_modules/.pnpm/rollup@4.13.0/node_modules/rollup/dist/es/shared/node-entry.js:12898:10)
    at convertProgram (file:///Users/adam.ayres/dev/buttaplz/node_modules/.pnpm/rollup@4.13.0/node_modules/rollup/dist/es/shared/node-entry.js:12218:12)
    at Module.setSource (file:///Users/adam.ayres/dev/buttaplz/node_modules/.pnpm/rollup@4.13.0/node_modules/rollup/dist/es/shared/node-entry.js:14042:24)
    at async ModuleLoader.addModuleSource (file:///Users/adam.ayres/dev/buttaplz/node_modules/.pnpm/rollup@4.13.0/node_modules/rollup/dist/es/shared/node-entry.js:18681:13)
 ELIFECYCLE  Command failed with exit code 1.
 ELIFECYCLE  Command failed with exit code 1.
 ELIFECYCLE  Command failed.

But guarding code with if (isServer) will work no matter what, right?

If I guard my callback in my formAction$ with a if (isServer) check then it prevents this issue.