-
Notifications
You must be signed in to change notification settings - Fork 10.1k
test #5407
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
Open
NikhilPatil24
wants to merge
5,746
commits into
socketio:master
Choose a base branch
from
NikhilPatil24:main
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
test #5407
+122,693
−22,015
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Before this change, the session and user context were retrieved once per HTTP request and not once per session.
This change is necessary to be able to write "import type { ... }".
When compiling with TypeScript with module set to "node16" and moduleResolution to "node16", the following error would be thrown: > node_modules/engine.io-parser/build/cjs/index.d.ts:6:54 - error TS2304: Cannot find name 'TransformStream'. > 6 export declare function createPacketEncoderStream(): TransformStream<Packet, any>; > ~~~~~~~~~~~~~~~ > node_modules/engine.io-parser/build/cjs/index.d.ts:7:96 - error TS2304: Cannot find name 'TransformStream'. > 7 export declare function createPacketDecoderStream(maxPayload: number, binaryType: BinaryType): TransformStream<Uint8Array, any>; > ~~~~~~~~~~~~~~~ > Found 2 errors in the same file, starting at: node_modules/engine.io-parser/build/cjs/index.d.ts:6 This is because the TransformStream object is not exposed in the global scope in the `@types/node` package, even though it is since Node.js `v18.0.0`. Reference: https://nodejs.org/api/webstreams.html#class-transformstream Note: we only import the TransformStream type (not value) because it isn't defined on older Node.js versions. Related: - https://github.com/socketio/engine.io-parser/issues/136 - socketio/socket.io-client#1606
Imported from https://github.com/socketio/socket.io-redis-adapter/blob/ef5f0da0b4928fd422afc985aec0e233d34400c0/lib/cluster-adapter.ts This abstract class is currently used by the sharded Redis adapter. We import it here because the logic can be reused by the other adapters, which will then only need to handle the pub/sub mechanism.
The clustered adapter will now: - periodically check if a node has left the cluster - send an event when it leaves the cluster This should reduce the number of "timeout reached: only x responses received out of y" errors.
The Redis adapter is currently the only adapter which makes a distinction between publishing messages (one channel for all) and responses (one channel for each node).
Note: the current API does not currently allow the user to handle those
errors (and retry, for example). This might be worth investigating for
the next major version, maybe something like:
```js
try {
await io.emit("hello");
} catch (e) {
// something went wrong
}
```
Related: socketio/socket.io-mongo-adapter#15
This reverts commit 7427109. The new version of the `cookie` package contains code with optional chaining (`?.`), which is not supported by older Node.js versions (< 14). The types for cookie are now bundled, so that there is no conflict with the types coming from `cookie@1`: > error TS2724: '"cookie"' has no exported member named 'CookieSerializeOptions'. Did you mean 'SerializeOptions'? > > import type { CookieSerializeOptions } from "cookie"; > ~~~~~~~~~~~~~~~~~~~~~~ Related: socketio#5283
Passing { port: "443" } would include the port in the URL (":443").
The init() method of the adapter will now be called when creating a namespace with `io.of(<the-namespace>)`.
Note: any promise rejection is silently caught, as I don't see how we could properly expose the promise.
```js
const io = new Server({
adapter: myAdapter
});
// under the hood, this:
// - implicitly creates the main namespace (/)
// - creates an instance of `myAdapter` for the main namespace
// - calls `myAdapter.init()` (with this change)
```
Related:
- socketio#3662
- socketio/socket.io-postgres-adapter#16
The ClusterAdapter class has been moved to [1], so that this adapter only needs to implement to pub/sub mechanism. Also, [2] should reduce the number of "timeout reached: only x responses received out of y" errors, since the fetchSockets() requests will now succeed even if a server leaves the cluster. [1]: https://github.com/socketio/socket.io/tree/main/packages/socket.io-adapter [2]: socketio@0e23ff0
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The kind of change this PR does introduce
Current behavior
New behavior
Other information (e.g. related issues)