sidorares / node-mysql2

:zap: fast mysqljs/mysql compatible mysql driver for node.js
https://sidorares.github.io/node-mysql2/
MIT License
4.07k stars 619 forks source link

Documentation - add API Documentation for main public functions #985

Open michaelisvy opened 5 years ago

michaelisvy commented 5 years ago

hi, thanks a lot for the great work on mysql2!

When learning how to use mysql2, it took me a while to figure out how to call mysql2 from an automated test because I was not sure which method is asynchronous. Would it be useful to add an example like the below in the documentation?

const mysql = require('mysql2/promise');
describe('Simple database connection", () => {
    it('should do a plain select', async (done) => {
        const connection = await mysql.createConnection({ 
            host: 'localhost',
            user: 'michael',
            password: 'secret',
            database: 'mysongdb',
        });
        let [songs] = await connection.query('Select * from Song');
        connection.end();
        expect(songs[0].title).toEqual("Little Wing");
        done();
    });
});

If you are interested, I would be happy to do a pull request.

sidorares commented 5 years ago

I think it would be good to have full "formal" documentation, maybe in a style of https://github.com/mscdex/ssh2#api

BTW in your example no need for done() call - if your test case handler returns promise mocha will await for it. Also .end() is async

const mysql = require('mysql2/promise');
describe('Simple database connection", () => {
    it('should do a plain select', async () => {
        const connection = await mysql.createConnection({ 
            host: 'localhost',
            user: 'michael',
            password: 'secret',
            database: 'mysongdb',
        });
        let [songs] = await connection.query('select * from Song');
        expect(songs[0].title).toEqual("Little Wing");
       await connection.end();
    });
});
michaelisvy commented 5 years ago

thanks for the prompt feedback! So do you think it would make sense to add some examples to the Readme.md file? (which seems the closest to the example you've pointed to). And would you like me to send you a pull request?

Regarding the done(), thanks for your feedback. I have only tried with Jest/Jasmine and it seems to be mandatory there (I've tried the updated code you've sent me and the test fails). I'll try using Mocha and see how it goes.

sidorares commented 5 years ago

not sure about jasmine but Jest definitely awaits async handlers - https://jestjs.io/docs/en/tutorial-async#async-await

sidorares commented 5 years ago

There is already example here - https://github.com/sidorares/node-mysql2/blob/master/documentation/Promise-Wrapper.md#es7-async-await

I'm more in favour of complete api doc ( listing all methods with input / result types )

michaelisvy commented 5 years ago

indeed, the examples are what I was in need of. Do you think it would make sense to add links to examples from the Readme?

Also agreed about the API Doc, that would help a lot. Do you have any solution in mind? May be JSDoc?

michaelisvy commented 5 years ago

Actually I've looked into JSDoc a bit and it seems that it requires all functions to have some comments on top. That would require significant refactoring...

sidorares commented 5 years ago

Surface api is relatively small, we can start fully manually or just document public methods in the code and add auto-generation script

michaelisvy commented 5 years ago

I've started playing a bit with JSDoc. I can keep working on a suitable JSDoc config if you'd like.

Do you think it's a suitable framework, or would you like to benchmark alternatives first? I'm fairly new to Javascript. Based on what I've seen on the web, JSDoc seems quite popular but I could be wrong :).