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

CJS dependencies #429

Open
dretsa opened this issue Jul 15, 2024 · 6 comments
Open

CJS dependencies #429

dretsa opened this issue Jul 15, 2024 · 6 comments
Labels
bug Something isn't working

Comments

@dretsa
Copy link

dretsa commented Jul 15, 2024

Describe the bug
While the framework itself is ESM, there are some dependencies in the proejct which are CJS or also have ESM capabilities but they are not properly annotated.

The packages I've identified are:

To Reproduce

Steps to reproduce the behavior:

  1. Git clone https://github.com/ripejs/core
  2. Run yarn
  3. Run yarn workspace @ripejs/framework build
  4. Remove external in packages/example/rollup.config.mjs
  5. Run yarn workspace @ripejs/example build

Expected behavior

Dependencies should be ESM compatible so they can be tree-shaken.

Versions

  • node: v22.2.0
  • @tinyhttp/app: 2.2.4
  • @tinyhttp/logger: 2.0.0

Additional context

I want to create a small opinionated ESM-only framework.

The idea is to create a tree-shakeable app which is bundled into a small js file.

@dretsa dretsa added the bug Something isn't working label Jul 15, 2024
@talentlessguy
Copy link
Member

talentlessguy commented Jul 15, 2024

CommonJS, if it doesn't have side effects, can be tree-shaken as well.

also, some of those deps don't have an ESM alternative, so I am not really sure what to do about it. I could maintain all the ES fork of all the deps that tinyhttp has, but then it would add additional maintenance burden and would require effort to keep up with bugfixes of the upstream.

@dretsa
Copy link
Author

dretsa commented Jul 15, 2024

That's something I didn't really know about cjs! I also thought it would be nice to only use one module resolution strategy, instead of mixing multiple. I have been using NodeJS 22 for a while and it seems like ESM module resolution is not experimental anymore.

I wonder how active the upstreams are and if I was to submit a PR they could review and merge?

@talentlessguy
Copy link
Member

Can we close this? It's not a bug, neither it breaks anything. You're not supposed to bundle deps for a backend framework

@dretsa dretsa closed this as completed Sep 12, 2024
@dretsa
Copy link
Author

dretsa commented Oct 8, 2024

@talentlessguy I have forked and ported the negotiator library to ESM and TypeScript - see jshttp/negotiator#71

Would you be open to change to this package if they don't merge it into the main?

You're not supposed to bundle deps for a backend framework

There are cases where you'd want to bundle like a serverless environment. Cold boots can be really slow if you have a lot of files. For example, in one of my bigger projects a cold boot was ~7s, down to ~1.5s after bundling.

Even AWS suggest bundling - see here.

@dretsa dretsa reopened this Oct 8, 2024
@talentlessguy
Copy link
Member

talentlessguy commented Oct 8, 2024

@colorninja didn't think about serverless. Yeah I could fork negotiator and move it under the tinyhttp org

@dretsa
Copy link
Author

dretsa commented Oct 9, 2024

I've ported it to ESM and created an npm library to test with: https://www.npmjs.com/package/negotiator-es

Also tried to replace the usages in tinyhttp with the ESM version (^0.1.6) and all tests run fine :)

Shall I create a PR to move it under the monorepo? There is more work needed on types but it has slightly more types than @types/negotiator. I didn't really change the code, just adapted the tests to run in node test runner

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants