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

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

Closed
na-- opened this issue Feb 22, 2019 · 22 comments
Closed

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

na-- opened this issue Feb 22, 2019 · 22 comments

Comments

@na--
Copy link
Member

na-- commented Feb 22, 2019

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
Copy link
Contributor

ppcano commented Apr 15, 2019

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

  • The crypto module is not working.
  • There are missing modules: k6/encoding, k6/metrics, k6/html and k6/ws.

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--
Copy link
Member Author

na-- commented Jun 6, 2019

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

@bookmoons
Copy link
Contributor

I'm working toward this in bookmoons/k6.

@bookmoons
Copy link
Contributor

Made a submission to DefinitelyTyped DefinitelyTyped/DefinitelyTyped#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
Copy link
Contributor

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--
Copy link
Member Author

na-- commented Jun 27, 2019

Thanks! 🎉 I skimmed through the PR, and yeah, the type system seems... interesting 😄

@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
Copy link
Contributor

Great! 🎉

I think that we should do both ;)

@robingustafsson
Copy link
Member

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

@na--
Copy link
Member Author

na-- commented Jun 27, 2019

Created #1063 😄 Closing this, since it seems done.

@na-- na-- closed this as completed Jun 27, 2019
@bookmoons
Copy link
Contributor

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

@ppcano
Copy link
Contributor

ppcano commented Jun 27, 2019

@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

  1. 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
Copy link
Contributor

Nice bug report. Will look into it.

@robingustafsson
Copy link
Member

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

@na--
Copy link
Member Author

na-- commented Jun 28, 2019

@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
Copy link
Contributor

bookmoons commented Jun 28, 2019

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
Copy link
Contributor

ppcano commented Jul 1, 2019

@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
Copy link
Contributor

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

@bookmoons
Copy link
Contributor

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
Copy link
Contributor

That update has been published DefinitelyTyped/DefinitelyTyped#36586 (comment)

@ppcano
Copy link
Contributor

ppcano commented Jul 5, 2019

@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
Copy link
Contributor

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--
Copy link
Member Author

na-- commented Jul 8, 2019

Closing this, since the type definitions are done. Opened #1071 to track the auto-import improvements.

@na-- na-- closed this as completed Jul 8, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants