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

fix: don't load websockets w/o crypto support #1868

Merged
merged 2 commits into from
Jan 19, 2023

Conversation

KhafraDev
Copy link
Member

No description provided.

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@mcollina mcollina merged commit 2d2512c into nodejs:main Jan 19, 2023
@KhafraDev KhafraDev deleted the crypto-without-ssl branch January 19, 2023 20:29
@jimmywarting
Copy link
Contributor

jimmywarting commented Jan 19, 2023

Q: Dose web crypto come with every new-ish NodeJS version? is it only node:crypto that lands in some build versions?

Follow up Q (if web crypto is available in all builds): Can we use web crypto instead whenever it's available and perhaps fallback to node:crypto instead if web crypto is not available in NodeJS version x.x.x?

@mcollina
Copy link
Member

If there is no crypto it's because Node was built without OpenSSL. This is a remote case, but we also test this in Node CI.

@mcollina
Copy link
Member

@KhafraDev why are we building Websocket in the Nodejs bundle? We don't use it there (yet).

@KhafraDev
Copy link
Member Author

KhafraDev commented Jan 19, 2023

@mcollina we aren't, I don't really know why it's being imported. Thinking it over, the issue might not be fixed since the node bundle shouldn't be using index.js?

This is the file that node should be using: https://github.com/nodejs/undici/blob/main/index-fetch.js, it doesn't import websocket anywhere. Nothing else should be using the websocket utils either.

@merceyz
Copy link
Member

merceyz commented Jan 19, 2023

I don't really know why it's being imported

The esbuild bundle analyser says the following:

lib/websocket/websocket.js
This file is included in the bundle because:

Entry point index-fetch.js contains:
require("./lib/fetch");

Required file lib/fetch/index.js contains:
require("../../index");

Required file index.js contains:
require("./lib/websocket/websocket");

So required file lib/websocket/websocket.js is included in the bundle.

#1869 removes it from the bundle.

anonrig pushed a commit to anonrig/undici that referenced this pull request Apr 4, 2023
metcoder95 pushed a commit to metcoder95/undici that referenced this pull request Jul 21, 2023
crysmags pushed a commit to crysmags/undici that referenced this pull request Feb 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants