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

Server runtime: remove need for node compat layer (=> removing 200k from worker bundle size) #445

Closed
brillout opened this issue Sep 9, 2022 · 12 comments
Labels
enhancement ✨ New feature or request

Comments

@brillout
Copy link
Member

brillout commented Sep 9, 2022

Description

We can make the server runtime completely agnostic to Node.js. Removing the need for Node.js shims on Edge platforms.

@brillout brillout added the enhancement ✨ New feature or request label Sep 9, 2022
@brillout
Copy link
Member Author

brillout commented Nov 2, 2022

If someone wants/needs this, PM on discord.

@zeyuri
Copy link

zeyuri commented Dec 26, 2022

I would be interested in doing this

@brillout
Copy link
Member Author

I would be interested in doing this

👍 PR welcome.

@zeyuri
Copy link

zeyuri commented Dec 27, 2022

Can you help me a little bit giving some guidance? How I have some experience porting some code from node to workers but I will be a little lost trying to find where this parts of the code are.

Also how would we make this work to not be a breaking change? A flag passed to the ssr plugin setup inside the vite.config.ts ?

@brillout
Copy link
Member Author

I believe we can replace all runtime Node.js dependencies with our own functions (self written or copied from e.g. StackOverflow). I think it's fairly straigthforward (although I may miss something). Last time I checked there were only few trivial Node.js dependencies. Note that it's only about the dependencies of the runtime code, not the entire VPS code.

With that approach we don't need any new VPS flag.

I'd run the Cloudflare example (/examples/cloudflare) without the Node.js compat flag and fix the errors until it works.

@brillout
Copy link
Member Author

brillout commented Apr 4, 2023

If I'm not mistaken all server runtime dependencies now don't depend on Node.js anymore. So it should be fairly easy to implement this now.

@magne4000
Copy link
Member

Perhaps some eslint config would also be welcome in order to not mistakenly add back some node dependency.

@brillout
Copy link
Member Author

brillout commented Apr 5, 2023

Yea, I've been thinking about adding eslint. Although note that this is about the server runtime. VPS's Vite plugin, for example, will still use Node.js.

But we can use the Cloudflare Workers tests to ensure that we don't mistakenly add back a Node.js dependency.

@kochrt
Copy link
Contributor

kochrt commented Apr 7, 2023

I'm mildly curious about this so I am fiddling around with it a bit. Not sure how far I'll make it so wanted to document in case I drop off and someone wants to pick up.

I'd run the Cloudflare example (/examples/cloudflare) without the Node.js compat flag and fix the errors until it works.

Seems the initial errors are all about path:

[ERROR] Could not resolve "path"

    node_modules/@brillout/import/dist/index.js:8:39:
      8 │ const path_1 = __importDefault(require("path"));~~~~~~

  The package "path" wasn't found on the file system but is built into node.
  Add "node_compat = true" to your wrangler.toml file to enable Node compatibility.


✘ [ERROR] Could not resolve "path"

    node_modules/vite-plugin-ssr/dist/cjs/node/runtime/renderPage/getPageAssets.js:10:39:
      10 │ const path_1 = __importDefault(require("path"));
         ╵                                        ~~~~~~

  The package "path" wasn't found on the file system but is built into node.
  Add "node_compat = true" to your wrangler.toml file to enable Node compatibility.


✘ [ERROR] Could not resolve "path"

    node_modules/vite-plugin-ssr/dist/cjs/utils/getOutDirs.js:10:39:
      10 │ const path_1 = __importDefault(require("path"));~~~~~~

  The package "path" wasn't found on the file system but is built into node.
  Add "node_compat = true" to your wrangler.toml file to enable Node compatibility.


✘ [ERROR] Could not resolve "path"

    node_modules/vite-plugin-ssr/dist/cjs/utils/moduleExists.js:4:23:
      4 │ const path_1 = require("path");
        ╵                        ~~~~~~

  The package "path" wasn't found on the file system but is built into node.
  Add "node_compat = true" to your wrangler.toml file to enable Node compatibility.

@brillout
Copy link
Member Author

brillout commented Apr 7, 2023

@kochrt How about replacing these path imports with a path polyfill and see whether Cloudflare still complains?

(Note that not all path imports need be removed but only the ones from the server runtime.)

@kochrt
Copy link
Contributor

kochrt commented Apr 8, 2023

Using pathe in conjunction with the pull request in @brillout/import seems to eliminate errors in the vue example.

@brillout
Copy link
Member Author

brillout commented Apr 9, 2023

Done, released as 0.4.112. Huge props to @kochrt 💯.

On a tangent: in case someone is able to convince his employer: https://github.com/sponsors/brillout. Sponsors are making a big difference!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement ✨ New feature or request
Projects
None yet
Development

No branches or pull requests

4 participants