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

docs: introduce TypeScript documentation #2122

Merged
merged 25 commits into from
Jul 9, 2023

Conversation

wellwelwel
Copy link
Sponsor Collaborator

@wellwelwel wellwelwel commented Jul 8, 2023

I added an installation case for @types/node in README.md, but described its purpose in /documentation/en/TypeScript-Examples.md.

Every example in this documentation was tested 🤹🏻‍♀️

You can follow the documentation here.


Annotations:

  • Create practical examples based on Issues in examples/typescript/
  • Check spelling after completing the documentation

Every file that was changed has a direct relation to the documentation 🙋🏻‍♂️

@wellwelwel
Copy link
Sponsor Collaborator Author

wellwelwel commented Jul 8, 2023

After several tests, I never got an OkPacket response.
Also searching through the code, all the uses of OK that I found are only used internally.

Instead, every query always returns RowDataPacket[], RowDataPacket[][], ResultSetHeader or ResultSetHeader[].

Including query().on('result', (result) => {})

Examples from source code (JS)

Handling it considering not to cause a break change

  1. Set changedRows in ResultSetHeader as a required property

    Done ✅

  • JS
    • In /lib/packets/resultset_header.js:
      if (m !== null) { 
        this.changedRows = parseInt(m[1], 10); 
    + } else {
    +   this.changedRows = 0;
      }
  • TS
    • In typings/mysql/lib/protocol/packets/ResultSetHeader.d.ts:
    -   changedRows?: number;
    +   changedRows: number;
      }
    
      export { ResultSetHeader };
  1. Add ResultSetHeader[] possibility to query and execute (for multipleStatements)

    Done ✅

  2. Optional: comment in typings/mysql/lib/protocol/packets/OkPacket.d.ts

    If you agree with this, I will do it in another PR, after documentation

    + /** @deprecated */
      declare interface OkPacket {
    • Later, in a major version, remove the OkPacket and OkPacket[] from query and execute types.


That said, I don't propose to use the OkPacket type in TypeScript documentation.

For now, I will continue the TypeScript documentation, but ignoring the OkPacket.

@wellwelwel
Copy link
Sponsor Collaborator Author

wellwelwel commented Jul 8, 2023

The documentation and the examples are ready 🕺🏻

I have created seven TypeScript examples based on Issues.
Tomorrow, I will review them before completing this PR.

@wellwelwel wellwelwel merged commit 00508c3 into sidorares:master Jul 9, 2023
37 checks passed
@sidorares
Copy link
Owner

Thanks @wellwelwel!

I think the naming confusion comes from the fact it's called "OK_Packet" in the documentation and it's a ResultSetHeader here. We should probably only use ResultSetHeader name for consistency

changedRows is not defined at all in the protocol, but the property existed in mysqljs/mysql so we support it as well. It's basically just a "view" into a human-readable "info" part if the info contains "changed [number]" in it's text. I'm not sure if we should set it to 0 if it can't be parsed - wdyt @wellwelwel ?

@wellwelwel
Copy link
Sponsor Collaborator Author

wellwelwel commented Jul 9, 2023

Hey, @sidorares 🙋🏻‍♂️

I have chosen 0 to not cause a breaking change.

I did a quick test and even a personal project would break if I changed from number to another type, like false or null, for example.

About keep it as it is...

I explain it better in my previous comment and complementing, the reason for this is to pattern the main difference between OkPacket and ResultSetHeader, especially to be able to explain the ResultSetHeader and its usability, ignoring the OkPacket in TypeScript documentation.

Just complementing (again), basically the changedRows appears in ResultSetHeader when an UPDATE query is performed.

Feel free to change it, I'm always open for ideas and suggestions 🤹🏻‍♀️

@wellwelwel
Copy link
Sponsor Collaborator Author

wellwelwel commented Jul 9, 2023

For optional fields, especially in TypeScript, is recommended check if the field exists, and if yes, the changedRows expects for a number, if we change this:

  • It would work perfectly, because it check for type:
typeof myResultSetHeader?.changedRows === 'number';
  • Causing a breaking change:
'changedRows' in myResultSetHeader && myResultSetHeader.changedRows > 0;

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

Successfully merging this pull request may close these issues.

2 participants