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

Add generated protocol types #1280

Closed
wants to merge 3 commits into from
Closed

Conversation

zardoy
Copy link
Contributor

@zardoy zardoy commented Jan 10, 2024

Add intellisense for packets:

Unfortunately in TS no way to display optionality or jsdoc for overloaded methods, its only possible with js proxied objects.

Alternatively, data can be generated in minecraft-data and reused here as type.

If everything is ok, I'll add the same for events (receive from server data)

const fs = require('fs')
const path = require('path')
const mcData = require('minecraft-data')
// const dataPaths = require('../minecraft-data/data/dataPaths.json')
Copy link
Member

Choose a reason for hiding this comment

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

commented code should be removed


const packetKeys = []
// from client
const packets = Object.entries(protocol.play.toServer.types).map(([name, [type, props]]) => {
Copy link
Member

Choose a reason for hiding this comment

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

this code should live in node-protodef

Copy link
Contributor Author

Choose a reason for hiding this comment

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

but noprotodefde-protodef doesn't have the minecraft-data dependency, what's your proposing workflow?

Copy link
Member

Choose a reason for hiding this comment

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

I think the code generating typescript types for protodef types should be generic to any protodef type.
Node protodef would then expose a function for generating such typescript types and then node-minecraft-protocol would use it

@@ -0,0 +1,134 @@
const fs = require('fs')
Copy link
Member

Choose a reason for hiding this comment

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

this logic should be tested, there are a lot of edge cases so it's probably not fully working as is, tests will make it possible to fix it

node protodef has test values

@rom1504
Copy link
Member

rom1504 commented Feb 26, 2024

thanks for the pr; but as said in comments this should live in node-protodef and be more generic

node-minecraft-protocol relies on node-protodef for the definition and implementation of its serialization functions

@rom1504 rom1504 closed this Feb 26, 2024
@zardoy
Copy link
Contributor Author

zardoy commented Feb 26, 2024

I'm really interested in this. I do hope to not forget to do this, The last time I looked at it I realized I need more time to implement it there... for now I will just use generated files: https://github.com/zardoy/prismarine-web-client/blob/d36bec02bfb7222ef8ea1b0503371bf16812a9e2/src/generatedServerPackets.ts its really handy

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Closed
Development

Successfully merging this pull request may close these issues.

2 participants