emit noAuth event #1283
emit noAuth event #1283
Conversation
| @@ -51,6 +51,7 @@ declare module 'minecraft-protocol' { | |||
| on(event: `raw.${string}`, handler: (buffer: Buffer, packetMeta: PacketMeta) => PromiseLike): this | |||
| on(event: 'playerChat', handler: (data: { formattedMessage: string, message: string, type: string, sender: string, senderName: string, senderTeam: string, verified?: boolean }) => PromiseLike): this | |||
| on(event: 'systemChat', handler: (data: { positionId: number, formattedMessage: string }) => PromiseLike): this | |||
| on(event: 'noCredentials', handler: () => PromiseLike): this | |||
There was a problem hiding this comment.
shouldnt it also use typed event emitter as mineflayer does?
| @@ -24,6 +24,7 @@ module.exports = function (client, options) { | |||
| } else { | |||
| if (packet.serverId !== '-') { | |||
There was a problem hiding this comment.
btw looking at the code, what's the point of this check? so the response is sent anyway below...
There was a problem hiding this comment.
What is the purpose of your PR ? What does this do?
There was a problem hiding this comment.
It was inspired by the pakkit patch, in my case it was also useful to have in the web app because I was not even getting an answer back from the server, so I was using this event to display an error message to the user
There was a problem hiding this comment.
It's not generally a good idea to make a change to code without understanding what's actually happening.
The creation of this event implies there is actually some credential issue here, can you look into what's happening here?
There was a problem hiding this comment.
It's not generally a good idea to make a change to code without understanding what's actually happening.
Yes, you are right, I just saw the message in the debug console and decided to also put an event there. But doesn't it mean what it says?
The creation of this event implies there is actually some credential issue here, can you look into what's happening here?
WDYM?
Do you mean there is an issue with my application instead?
There was a problem hiding this comment.
My goal is to end the connection as soon as I understand that the server is running online mode (and display it in UI)
|
btw let's add minimal esbuild web example here (I want to pick some other changes as well) |
| @@ -274,6 +274,10 @@ Emitted after the player enters the PLAY protocol state and can send and recieve | |||
|
|
|||
| Called when an error occurs within the client. Takes an Error as parameter. | |||
|
|
|||
| ### `noCredentials` event | |||
|
|
|||
| Emitted when client didn't supply credentials and server is in online mode (authenticating is required). | |||
There was a problem hiding this comment.
Im not sure if appears needs to be added here. Is it possible to encounter a case where noCredentials would be emitted and the connection is successful?
There was a problem hiding this comment.
Do you have a link to nmp/vanilla code or somewhere to doc how this gets triggered? I don't understand how packet.serverId relates to auth.
There was a problem hiding this comment.
I don't understand how packet.serverId relates to auth.
i don't know why there is packet.serverId check, sorry. That's why I raised the question below. Or did you mean something else? I wish we could improve clarity here. This is what I want to get: https://github.com/PrismarineJS/node-minecraft-protocol/pull/1283/files#r1454466037
There was a problem hiding this comment.
Yes, I mean we should not be formally emitting something like "no credentials" without being certain there is some credential issue and what's causing this. Usually this is what unit tests are supposed to catch. I don't think we need tests for this change, but an understanding of from where this issue originates, not just some debug comment.
There was a problem hiding this comment.
Yes, I mean we should not be formally emitting something like "no credentials" without being certain there
Ok, I see your point. I have only my personal experience: whenever I try to connect to any online server I get it.
Not sure what to do here for now..
|
the comments seem to indicate this PR is not the right fix. Please re-open if you want to find an other path @zardoy |
...so its possible to interrupt the client sending anything e.g.