grafana / k6

A modern load testing tool, using Go and JavaScript - https://k6.io
GNU Affero General Public License v3.0
25.04k stars 1.24k forks source link

Update the type definitions for the k6/* APIs #929

Closed na-- closed 5 years ago

na-- commented 5 years ago

As @marklagendijk noted in slack, there's apparently a types package for k6: https://www.npmjs.com/package/@types/k6 (the actual files are located here). According to him it's not 100% complete, so it has to be checked and likely slightly improved.

We should probably also try to make sure we update those definitions if we add new k6 APIs... Not sure how possible it would be to automate that task, as some sort of a new CI check or unit test that complains if something is missing or mismatched... Likely not easy at all, so we may have to be satisfied with a single reminder in CONTRIBUTING.md stating something like "if you're changing the APIs, make sure you update this repo as well"...

ppcano commented 5 years ago

I tried it using npm install --save-dev @types/k6 and it looks the existing TypeScript definition:

The project looks a good start and it should not be difficult to iterate it. I am in favor of completing the k6 TypeScript definition; vscode users will benefit from it, and video tutorials will look like better.

Screen Shot 2019-04-15 at 7 28 22 AM Screen Shot 2019-04-15 at 7 28 06 AM
na-- commented 5 years ago

It seems like we'd have to more carefully comb through the existing type definitions, since they contain some inaccuracies. As mentioned in https://github.com/loadimpact/k6/issues/1041#issuecomment-499364250, timings.looking_up is present in the user-contributed k6 type definitions, while it was removed in k6 v0.13 (more info).

bookmoons commented 5 years ago

I'm working toward this in bookmoons/k6.

bookmoons commented 5 years ago

Made a submission to DefinitelyTyped https://github.com/DefinitelyTyped/DefinitelyTyped/pull/36432 . Crazy type system in TypeScript.

A little example code.

/// <reference types="k6" />

import { Response, get } from 'k6/http'
import { Selection, TitleElement } from 'k6/html'

export default function () {
    const response: Response = get('http://httpbin.org')
    const document: Selection = response.html()
    const title: TitleElement = document.find('title').get(0) as TitleElement
    console.log(title.text())
}
bookmoons commented 5 years ago

Looks like these types were quickly reviewed and merged DefinitelyTyped/DefinitelyTyped#36432. The npm package is already published: https://www.npmjs.com/package/@types/k6

The reviewer had a nonblocking concern. Opened DefinitelyTyped/DefinitelyTyped#36473 to address it.

na-- commented 5 years ago

Thanks! :tada: I skimmed through the PR, and yeah, the type system seems... interesting :smile:

@MStoykov, @bookmoons, @robingustafsson: do you think it's worth adding a line in CONTRIBUTING.md to update these definitions when API changes are made? Or is it time to add a pull request template? Or none of the above, and just deal with the rare API changes individually in the code reviews?

mstoykov commented 5 years ago

Great! :tada:

I think that we should do both ;)

robingustafsson commented 5 years ago

Agree with @MStoykov, let's do both :)

na-- commented 5 years ago

Created https://github.com/loadimpact/k6/issues/1063 :smile: Closing this, since it seems done.

bookmoons commented 5 years ago

Thanks guys. Looks like that final update DefinitelyTyped/DefinitelyTyped#36473 was also merged.

ppcano commented 5 years ago

@bookmoons, thank you for working on this issue.

I have tested how vscode works when reading the Type definitions, and I found:

1) auto-import is not working for all the modules. The gif below shows how it works for the k6 module like the sleep function, but it does not on other modules.

2019-06-27 22 46 19

2) I think it would be useful if the Type definitions can also include the documentation.

2019-06-27 22 45 03

@na, we have previously talked about the challenges of having the docs up-to-date.

bookmoons commented 5 years ago

Nice bug report. Will look into it.

robingustafsson commented 5 years ago

Re-opening issue to be able to post the bounty 🙂

na-- commented 5 years ago

@ppcano yeah, I guess it makes sense to at least have short descriptions of some of the most used functions, maybe as a separate issue/bounty? But as you know, I'm very leery on further fragmentation of the k6 docs - having the same things in multiple places makes synchronization super difficult...

bookmoons commented 5 years ago

I think this is a limitation of the autoimport extension. Tried it with another structured package and it also doesn't seem to find things.

npm install --save @types/d

There's a function d under require('d') that gets found. There's a function lazy under require('d/lazy') that isn't found. Tried it with the most used extension https://github.com/soates/Auto-Import . If you want this enough I could pursue a patch to the extension.

I could take up adding descriptions under an enhancement issue. Wonder if there's a way to generate docs from the declaration files. Get it all in one place.

ppcano commented 5 years ago

@bookmoons For context, I don't know much about Type definitions and vscode but I reviewed it a bit for suggesting this feature.

There were a few autoimport plugins but vscode supports Auto Import by default since Oct 2017/V1.18.

"configuration.suggest.autoImports": "Enable/disable auto import suggestions. Requires using TypeScript 2.6.1 or newer in the workspace.",

First, I used auto-import on different modules when using the Ember Type definition and it works with classes Controller, Component ... or functions like capitalize.

npm install --save-dev @types/ember

There are no many Type definitions with multiple namespace/modules, but ol is another one:

npm install --save-dev @types/ol

import TileGrid from "ol/tilegrid/TileGrid";
import TileSource from "ol/source/Tile";
import VectorSource from "ol/source/Vector";

I can help with the testing or any other thing.

bookmoons commented 5 years ago

Will look into this. That ol package is a really good example.

bookmoons commented 5 years ago

It seems autoimport only sees things that are loaded by the index file. Adding simple imports for everything to index makes it work.

import './crypto';
import './encoding';
..

Opened DefinitelyTyped/DefinitelyTyped#36586 to deploy this.

bookmoons commented 5 years ago

That update has been published https://github.com/DefinitelyTyped/DefinitelyTyped/pull/36586#issuecomment-507747437

ppcano commented 5 years ago

@bookmoons Please, could you check the Type definition of the default module for the http, ws and crypto APIs.

// auto-imported
import { get } from "k6/http";

// not auto-imported
import http from "k6/http";

2019-07-05 10 17 27

bookmoons commented 5 years ago

Had a look at this. Think I'd have to do it under another issue.

I think there's a way to make it happen but I'd have to investigate to find something that works with TypeScript and DefinitelyTyped conventions and doesn't break the destructuring imports. Potentially restructure the modules to make it possible.

na-- commented 5 years ago

Closing this, since the type definitions are done. Opened https://github.com/loadimpact/k6/issues/1071 to track the auto-import improvements.