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

deps: bundle full sources of undici and not just precompiled blob #42199

Closed
AdamMajer opened this issue Mar 3, 2022 · 9 comments
Closed

deps: bundle full sources of undici and not just precompiled blob #42199

AdamMajer opened this issue Mar 3, 2022 · 9 comments
Labels
feature request Issues that request new features to be added to Node.js.

Comments

@AdamMajer
Copy link
Contributor

What is the problem this feature will solve?

Current Nodejs is bundling undici but instead of the sources, we only have a pre-compiled blob.

What is the feature you are proposing to solve the problem?

Bundle the entire indici snapshot in nodejs versions. In sources maybe this could be done with git submodules but also just an outright copy of specific checkout.

What alternatives have you considered?

No response

@AdamMajer AdamMajer added the feature request Issues that request new features to be added to Node.js. label Mar 3, 2022
@targos
Copy link
Member

targos commented Mar 3, 2022

What would be the advantage(s) of doing it?

@AdamMajer
Copy link
Contributor Author

For distributions, we always like to have the sources available so we can build from them. For example, in the future maybe a fix needs to be applied and just upgrading the entire package to the latest version may be no longer compatible.

I guess for Debian, the bundled .js + wasm is not even distributable since it's not preferred source format for modifications.

@richardlau richardlau changed the title deps: bundle full sources of indici and not just precompiled blob deps: bundle full sources of undici and not just precompiled blob Mar 3, 2022
@mhdawson
Copy link
Member

mhdawson commented Mar 3, 2022

I guess for Debian, the bundled .js + wasm is not even distributable since it's not preferred source format for modifications.

can yo clarify what you mean by that?

@AdamMajer
Copy link
Contributor Author

AdamMajer commented Mar 4, 2022 via email

@benjamingr
Copy link
Member

I think this is a reasonable ask to make floating patches easier but I'm hesitant to sacrifice the developer experience somewhat for it.

For what it's worth and just to be clear undici is open source too and you can change stuff in the "wasm bits" (llhttp, another dep that is also open source), build it and then tell Node.js to use it by updating undici in the deps folder.

Would a process that makes that change easier help or is avoiding having any blobs the important bit?

Note (funnily) Node.js already builds llhttp since it also uses it internally, I wonder how much work it would be to just get that to produce the wasm for undici.

@dnlup worked on the wasm build if I am reading the llhttp repo correctly, maybe he can weigh in?

targos added a commit to targos/node that referenced this issue Mar 7, 2022
This also adds a script to automate the update and includes the sources
included in the npm tarball.

Closes: nodejs#42199
@targos
Copy link
Member

targos commented Mar 7, 2022

I opened #42246 which includes a copy of the undici sources in deps/undici/src.

@mhdawson
Copy link
Member

mhdawson commented Mar 7, 2022

@AdamMajer does #42246 address your concern?

@AdamMajer
Copy link
Contributor Author

@mhdawson yes, thank you.

@mhdawson
Copy link
Member

mhdawson commented Mar 9, 2022

@AdamMajer thanks, great to hear wanted to be sure there were not other concerns.

@aduh95 aduh95 closed this as completed in cb3ce9f Mar 9, 2022
bengl pushed a commit that referenced this issue Mar 21, 2022
This also adds a script to automate the update and includes the sources
included in the npm tarball.

PR-URL: #42246
Fixes: #42199
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Mestery <[email protected]>
Reviewed-By: Robert Nagy <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Tierney Cyren <[email protected]>
aduh95 pushed a commit to aduh95/node that referenced this issue Apr 14, 2022
This also adds a script to automate the update and includes the sources
included in the npm tarball.

PR-URL: nodejs#42246
Fixes: nodejs#42199
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Mestery <[email protected]>
Reviewed-By: Robert Nagy <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Tierney Cyren <[email protected]>
aduh95 pushed a commit to aduh95/node that referenced this issue Apr 20, 2022
This also adds a script to automate the update and includes the sources
included in the npm tarball.

PR-URL: nodejs#42246
Fixes: nodejs#42199
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Mestery <[email protected]>
Reviewed-By: Robert Nagy <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Tierney Cyren <[email protected]>
danielleadams pushed a commit that referenced this issue Apr 21, 2022
This also adds a script to automate the update and includes the sources
included in the npm tarball.

PR-URL: #42246
Backport-PR-URL: #42727
Fixes: #42199
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Mestery <[email protected]>
Reviewed-By: Robert Nagy <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Tierney Cyren <[email protected]>
xtx1130 pushed a commit to xtx1130/node that referenced this issue Apr 25, 2022
This also adds a script to automate the update and includes the sources
included in the npm tarball.

PR-URL: nodejs#42246
Fixes: nodejs#42199
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Mestery <[email protected]>
Reviewed-By: Robert Nagy <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Tierney Cyren <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request Issues that request new features to be added to Node.js.
Projects
None yet
Development

No branches or pull requests

4 participants