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.

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

🐛 Bug: everything that's needed should be in the output directory #14

Open
dario-piotrowicz opened this issue Sep 20, 2024 · 7 comments
Assignees

Comments

@dario-piotrowicz
Copy link
Contributor

dario-piotrowicz commented Sep 20, 2024

The build does output a worker in the specified output directory (defaulting to .next-worker).

This directory should contain everything that's required to run the worker (assets included).

But this is not so, as there are clear signs that files within .next-worker make use of files outside of the directory (making the dev/deployment of unbundled workers (--no-bundle) impossible).

For example:

I'm not sure how those files are not getting bundled by the esbuild build step 😕

We should make sure that they are actually bundled in, or if that's not possible, everything that the worker requires should just need to live in the output directory and no external file/dependency should be needed (in other words, I think that we need to make sure that wrangler dev --no-bundle works).

@vicb
Copy link
Contributor

vicb commented Jan 22, 2025

@dario-piotrowicz I took a quick look at this and have some comments:

I'm not sure how those files are not getting bundled by the esbuild build step

At least part of why some file are not bundled by this pass is that they are not required.
You can see that patchRequire add requires after this first pass.

That is not the only patch adding deps that will be pick up by wrangler dev

I should tackle some of this over the next few days.

I think that we need to make sure that wrangler dev --no-bundle works

I think this is no more applicable.
The first pass of ESBuild is only for the server bundle.
OpenNext also generates a middleware bundle.
wrangler dev will stitch them together.

@vicb
Copy link
Contributor

vicb commented Jan 22, 2025

Is there a specific problem you think this was causing?

@dario-piotrowicz
Copy link
Contributor Author

Is there a specific problem you think this was causing?

yes, I am pretty sure #148 and #219 do require this to be fixed before we can properly address those

I bet there are other problems that can't be fixed without this addressed

If needed I can go back to the issues and provide here a more detailed explanation

@dario-piotrowicz
Copy link
Contributor Author

I think this is no more applicable. The first pass of ESBuild is only for the server bundle. OpenNext also generates a middleware bundle. wrangler dev will stitch them together.

I completely do no understand this.... are you saying we will always need to rely on wrangler's bundling? if that's the case that doesn't sound right at all to me, the two different bundles should both live under the .open-next directory and be sticked together by imports + something like find additional modules, no? Relying on bundling would also mean that we can't rely on lazy loading in our worker, which as we saw with next-on-pages can significantly improve performance (which we most likely need (#183))

@vicb
Copy link
Contributor

vicb commented Jan 22, 2025

are you saying we will always need to rely on wrangler's bundling?

Hum... I would say "plan" rather than "need" here.
Note that users can use their own bundling or no bundling if they need for some restricted use cases.
Probably something to document when things become more stable.

the two different bundles should both live under the .open-next directory

That's .open-next/middleware and .open-next/server-function

be sticked together by imports

That's in worker.ts

something like find additional modules
Relying on bundling would also mean that we can't rely on lazy loading in our worker

I believe lazy loading is possible in bundling mode using find_additional_modules?

@dario-piotrowicz
Copy link
Contributor Author

dario-piotrowicz commented Jan 22, 2025

I believe lazy loading is possible in bundling mode using find_additional_modules?

I think we already discussed about this and I think we confuse each other because "wrangler bundling" is just a confusing term in my opinion.

When I talk about bundling I mean in the true/strict sense, as in the fact you have N different js/ts files (which might include dependencies) and they all become a single js file that you can then send to Cloudflare, that's what I believe we do need not to depend on.

Bundling in the sense that we perform transformations on files and do some per-file bundling and whatnot, but allow lazy loading thought find_additional_modules is completely fine by me and I don't see any issue with that, as long as the base_dir is the .open-next directory (which is the directory that find_additional_modules traverses: Cloudflare docs)

@vicb
Copy link
Contributor

vicb commented Jan 29, 2025

Hey @dario-piotrowicz,

When #295 is merged, you can generate meta file in debug mode. There is one file per bundle (i.e. one for the middleware and an other for the server) located alongside the output with a .meta.json extension.

You need to set OPEN_NEXT_DEBUG=1 to generate them as they are only generated in debug mode, i.e.

OPEN_NEXT_DEBUG=1 node /path/to/opennextjs-cloudflare/packages/cloudflare/dist/cli/index.js

I hope the meta files contain all the info you need. Let me know otherwise.

edit: I added a second commit to the PR and the cloudflare bundle (handler.mjs) has the .meta.json generated whether or not OPEN_NEXT_DEBUG=1 - you only need to set the debug mode to generate meta files for OpenNext bundles.

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

2 participants