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

Move to monorepo #2732

Open
ronag opened this issue Feb 11, 2024 · 34 comments
Open

Move to monorepo #2732

ronag opened this issue Feb 11, 2024 · 34 comments
Labels
enhancement New feature or request

Comments

@ronag
Copy link
Member

ronag commented Feb 11, 2024

This would solve...

Currently the undici package is quite heavy with a lot of stuff that you might not need, e.g. web api. Would be good to separate undici "core" into its own package.

The implementation should look like...

Use pnpm

@ronag ronag added the enhancement New feature or request label Feb 11, 2024
@metcoder95
Copy link
Member

I think @Ethan-Arrowood was managing this idea for a while, after the split of undici-types

@Ethan-Arrowood
Copy link
Collaborator

There's some old threads about monorepo ideas. Happy to revisit

@mcollina
Copy link
Member

I think it's inevitable that we adopt a monorepo at this point, this project has grown too big, and there are separate subsystems that could be used separately.

@Ethan-Arrowood
Copy link
Collaborator

Use pnpm

This is also my preference. Does anyone feel extra-strongly about using npm?

pnpm is now included in corepack so it should be very easy for everyone to use it without any additional install or set up (minus a corepack enable call).

@Ethan-Arrowood
Copy link
Collaborator

I will also start by experimenting with npm workspaces to start. They've supposedly come along way so maybe its worth trying again.

@mcollina
Copy link
Member

I've not been able to make npm workspace work in any reasonable way for OSS development

I think the most useful part would be if somebody could figure out how to set it up with changesets so that we could ship independent versions of the packages. Alternatively, they will all have the same version.

@Ethan-Arrowood
Copy link
Collaborator

I have experience with changesets. Do you want me to try npm+changesets or go straight to pnpm+changesets?

@Ethan-Arrowood
Copy link
Collaborator

Rough plan:

  1. Create PR that moves everything into packages/undici and documentation
    • No code will split up just yet
    • CI should run as normal
    • Docs site should work as normal
    • Other tooling should continue to work (build, commit hooks, etc.), and run from root as normal
    • This should only introduce npm workspaces. Changesets should not be required.
    • Even though the code will exist in packages/undici directory, the package should still be named and published as undici (for now).
    • (In nodejs/node) Update Node.js -> Undici vendor process

Step 1 is simply the first step in migrating to a monorepo. This will reduce impact on contributors and should not disrupt development.

  1. Code split (incremental)
    • Create new packages packages/core, packages/fetch, and packages/types
    • Start publishing packages as @undici/core, @undici/fetch, and @undici/types
    • Retain packages/undici as undici and make it roll-up and export all of the other packages
    • Introduce changesets for version and publishing management.
    • (Optional) Move linting and formatting to root

Step 2 serves as an incremental step to a true monorepo organization. By retaining undici package we don't have to affect Node.js or users. Everything will appear as normal.

  1. Migrate Node.js from undici to @undici/fetch

    • Make necessary change in nodejs/node to vendor @undici/fetch instead of undici
  2. More code splitting!

    • Intentionally not-well-defined at this time, consider other parts of Undici that we can split out into its own separate packages.

@mcollina
Copy link
Member

I'm +1

@metcoder95
Copy link
Member

LGTM 🚀

@Ethan-Arrowood
Copy link
Collaborator

I'll have a PR up for step 1 very soon. I'd like to land #2766 first as that will help simplify things.

@mcollina
Copy link
Member

My experience with npm workspaces is that they are not fit for purpose. Specifically, they are missing the workspace protocol, https://pnpm.io/workspaces#referencing-workspace-packages-through-aliases, which makes it impossible to maintain an OSS library without it. (Or I couldn’t figure out how to handle versioning).


@KhafraDev wdyt?

@Ethan-Arrowood
Copy link
Collaborator

I've found more success where versions are updated by a tool such as changesets. That said, switching to pnpm has other great benefits -- and everyone can do it since it's supported in corepack. Ill make sure to present options for us to consider

@Uzlopak
Copy link
Contributor

Uzlopak commented Feb 20, 2024

I dont like monorepos. They make managing code complicated. I would rather have one package released on npm with everything than having to manage many packages where each package has different versions.

@KhafraDev
Copy link
Member

KhafraDev commented Feb 20, 2024

Without hearing the benefits of having a monorepo, I can't make an argument against it, or agree to having one.

A monorepo won't make undici easier to maintain, it won't make undici easier to publish, and it won't make it easier to install. Is the only benefit bundle size? If so, undici has constantly grown and now has 7.2 million downloads a week, and we have not received a single complaint about bundle size. People who use @types/node aren't complaining about the added size of undici-types, even though most of the types aren't being used.

The entirety of undici is 1.24 MB. According to npm, fetch is 277 kB, cache is 25.6, and WebSocket is 57.5. If we were to split them into separate packages, undici core will still be ~880 kB. That is a menial difference when the docs being bundled are 183 kB, types are ~80 kB, and llhttp is 283 kB. I believe Matteo prefers that docs are included for them to be easy to access (same reason we don't use esbuild on undici and put a minified file on npm), but claiming that bundle size is too big while shipping a substantial amount of markdown files doesn't make sense to me.

I am willing to change my mind on this, if there is a noticeable improvement to any area of development. Let me know 😄!

@Uzlopak
Copy link
Contributor

Uzlopak commented Feb 20, 2024

Well, next undici version should be about 150 kb smaller. We can still remove some artifacts in llhttp and save about 8 kb and maybe the 1kb in for the MIT license in the fetch folder by giving credits for fetch to @Ethan-Arrowood in the main license file.

Maybe 74 kb if we drop the non-simd-version of llhttp.

@Ethan-Arrowood
Copy link
Collaborator

The biggest motivation for me is simplification. We have so much going on in this library now, splitting into at least core and fetch would be a developer experience improvement imo.

I think the benchmark and documentation split outs are still valuable even if we don't reorganize code.

@KhafraDev
Copy link
Member

fetch is contained to lib/fetch, the only difference would be that it would be moved to packages/fetch/lib and I couldn't directly require/import the files since they'd have to use workspace aliases. The code is very organized and easy to work on right now.

@Ethan-Arrowood
Copy link
Collaborator

Also cache, eventsource, and websocket that are all related to web platform APIs too, right?

I won't open a monorepo PR just yet. I'd like to split out other pieces first like benchmarks, docs, and examples. Maybe @ronag or @mcollina have other inputs regarding monorepo organization.

@KhafraDev
Copy link
Member

yes, those are all self-contained (and small relative to package size)

@ronag
Copy link
Member Author

ronag commented Feb 20, 2024

I would prefer to have all web api stuff in own thing.

@metcoder95
Copy link
Member

The biggest motivation for me is simplification. We have so much going on in this library now, splitting into at least core and fetch would be a developer experience improvement IMO.

+1

@KhafraDev
Copy link
Member

KhafraDev commented Feb 20, 2024

how would it improve developer experience?

If developer = maintainer, the act of maintaining the tooling offsets any simplification (I don't see how adding more tooling could simplify the development process). Being able to write pure javascript that works without tooling is a huge benefit here. Even 'simple' tooling like esbuild has caused us issues - imagine more complex tools! Personally, moving to a monorepo would break my development process.

If developer = user, having to install more packages and deal with a breaking change for a benefit no one here has expanded on doesn't seem like a good idea.

I'm not looking for an essay or a research study, just anecdotal evidence would be fine.

@ronag
Copy link
Member Author

ronag commented Feb 20, 2024

For me it would be easier not to deal with all the web api both in terms of browsing code and testing.

@Uzlopak
Copy link
Contributor

Uzlopak commented Feb 20, 2024

regarding testing - what is the issue with that? too slow?

@KhafraDev
Copy link
Member

web tests will still run in a monorepo though

@ronag
Copy link
Member Author

ronag commented Feb 21, 2024

Then I would suggest to split it into two different repos. undici core and undici web.

@Ethan-Arrowood
Copy link
Collaborator

We can leverage a tool like turborepo to make this as efficient as possible, but since web would depend on core, any change to core would result in running web too. But like if maybe pieces like the redirect handler or mocks lived outside of core, then changing those pieces would not require running core or web. Just think of it like a directed graph. It will depend on how we organize our modules.


I think the issue some of us have is the information density of the project. Sure the code lives in separate folders, but yet we still have a lot of it. It is difficult to know where to make changes, how to add new apis, etc. Anecdotally, I was confused for a bit what the cache directory contained, and why it wasn't a http cache solution that all of undici could use (like the idea shared in #2256).

Beyond that I have other related issues such as code quality, documentation, and testing organization that come to mind. I can empathize with @KhafraDev and @Uzlopak , and agree that just switching to a monorepo layout is not going to fix these issues directly. However, in my opinion, fixing things like that can be easier when there is less code density.

Having a minimal core package will enable us to slim down and simplify the core api to a point where it will also be simpler to document, test, type, and more. Then other pieces can build off of that and adopt the same simplicity in docs, tests, etc.

@Ethan-Arrowood
Copy link
Collaborator

I haven't thought too much about what a minimal @undici/core would contain. I find the current Dispatcher implementation to be confusing. The indirection in particular is the worst part (such as the makeDispatcher piece in index.js). This obviously was done for good reason (such as extending/implementing the interface in an easy way across the repo). But if for example these pieces lived in their own module, it could force us to come up with a better API for other pieces like the Mock interfaces or custom Handlers.

This is just an off-the-cuff idea, but I hope it helps provide a little more anecdotal evidence behind my support for a monorepo organization

@ronag
Copy link
Member Author

ronag commented Feb 21, 2024

I think the information density is an excellent description. Something been bothering me but I couldn't describe it. I think especially for third-party library implementers (such as my own nxt-undici and others such as elastic and possibly axios, got etc...) who only actual need or care about undici core. We/They have to navigate through a huge project just for the few things we need.

@ronag
Copy link
Member Author

ronag commented Feb 21, 2024

The indirection in particular is the worst part (such as the makeDispatcher piece in index.js).

makeDispatcher is not a core thing.

@ronag
Copy link
Member Author

ronag commented Feb 21, 2024

I haven't thought too much about what a minimal @undici/core would contain.

I have a pretty solid idea of what I would have in core:

lib/node/*
lib/llhttp/*
lib/core/*
lib/agent.js
lib/timers.js
lib/balanced-pool.js
lib/client.js
lib/dispatcher*
lib/pool*

ronag added a commit that referenced this issue Feb 21, 2024
ronag added a commit that referenced this issue Feb 21, 2024
ronag added a commit that referenced this issue Feb 21, 2024
ronag added a commit that referenced this issue Feb 21, 2024
ronag added a commit that referenced this issue Feb 21, 2024
ronag added a commit that referenced this issue Feb 21, 2024
ronag added a commit that referenced this issue Feb 21, 2024
ronag added a commit that referenced this issue Feb 21, 2024
ronag added a commit that referenced this issue Feb 21, 2024
ronag added a commit that referenced this issue Feb 21, 2024
@KhafraDev
Copy link
Member

Then I would suggest to split it into two different repos. undici core and undici web.

I'd prefer this over a monorepo.

Sure the code lives in separate folders, but yet we still have a lot of it. It is difficult to know where to make changes, how to add new apis, etc.

@Uzlopak was able to add EventSource without any help from me or anyone else on how to organize code. If we're talking about the lower leveled dispatcher api/mocks/ProxyAgent/etc., that simply isn't an issue with the web stuff and wouldn't be solved by moving it. Moving the web code to their own folder (in the PR linked here) will let people use tooling to ignore those files.

Not only that, others have made breaking changes relatively quickly lately (such as the removal of some dispatcher handler options and interceptors).

possibly axios, got etc...

axios and got both use node:http and axios will use XMLHttpRequest in browsers. If axios ever moved to fetch then they likely use undici's fetch instead of undici core (or just global fetch). I know this is very shitty proof but I looked at the most downloaded dependents of undici on npm and every single one of them used fetch. I would go so far as to say that undici.fetch is just as fundamentally important as undici.request, etc., as they both serve two entirely separate use cases in a similar api.

I haven't thought too much about what a minimal @undici/core would contain. I find the current Dispatcher implementation to be confusing. The indirection in particular is the worst part (such as the makeDispatcher piece in index.js). This obviously was done for good reason (such as extending/implementing the interface in an easy way across the repo). But if for example these pieces lived in their own module, it could force us to come up with a better API for other pieces like the Mock interfaces or custom Handlers.

I'm very confused as to how you came to the conclusion that the core api being confusing can be solved by moving the web stuff. The web stuff doesn't need docs, and this seems like a documentation issue w.r.t. core.

I can't speak on information density because I've never had issues with it honestly. Of course we could move MockAgent or ProxyAgent to their own packages, and I wouldn't have an issue with that, because they are higher leveled than even fetch, and do not rely on core.

Fetch relies on core internals and core relies on fetch internals (specifically undici.request w/ a FormData body). There is no easy way of decoupling fetch from core with a benefit that seems to boil down to '[core/web] is too [confusing/big]' ("big" in a more abstract way than package size). From what I've seen @Ethan-Arrowood mostly has an issue with documentation in core, because the web stuff simply has enough documentation on MDN or a million other places.

So I would like to leave a few questions:

@Ethan-Arrowood
Copy link
Collaborator

In short, yes many of these issues can be solved with better/more documentation AND improved code organization (not necessarily meaning monorepo). Let's start there and iterate.

crysmags pushed a commit to crysmags/undici that referenced this issue Feb 27, 2024
KhafraDev pushed a commit to KhafraDev/undici that referenced this issue Jun 24, 2024
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

6 participants