micmro / PerfCascade

Responsive, SVG based HAR waterfall viewer
https://micmro.github.io/PerfCascade
MIT License
276 stars 51 forks source link

Limit exported types? #128

Closed tobli closed 7 years ago

tobli commented 7 years ago

I don't have much experience with TypeScript, so this might be a misunderstanding. That said; I see the exported API via index.js is:

The types that can be passed to, or returned from, these methods are:

Thus all other types (e.g. HoverEvtListeners and RectData) could be considered implementation details. Therefor they don't need to be exported in the package "types". Especially if we intend to use semantic versioning it's beneficial to keep the exported surface to a minimum to have more freedom to make implementation detail changes.

ping @micmro

micmro commented 7 years ago

Right, I had meat to get back to this at some stage but forgot. Though we should export the types of the options and parameter as well, so user of the API can pass them around as well.

One usecase were the RectData etc would be usefull is when creating a new non-HAR adapter, but this this is negligible in the moment since we want to change the structure anyway.

micmro commented 7 years ago

I've started looking into this. I suggest to export additionally all har and waterfall types in separate namespaces to make the api easier to discover, to allow users to write new transformers or to build the har from scratch.

What do you think?

screen shot 2017-02-25 at 3 35 25 pm screen shot 2017-02-25 at 3 35 38 pm screen shot 2017-02-25 at 3 35 50 pm
gitgrimbo commented 7 years ago

Hi @micmro. Would you consider exporting your HAR typings as a standalone module in npm?

I'm writing some TypeScript code that deals with HAR files and your typings seem much more detailed than mine.

Thanks.

micmro commented 7 years ago

Hi Paul, sure, I can do that. Perhaps I can also add it to Definetly-Typed, if thats possible for generic typings.

tobli commented 7 years ago

Can we avoid exposing the internal Waterfall format? Locking it down to public api really limits how we're able to innovate/evolve the format. I'd assume the by far most common use case is just rendering an existing har.

micmro commented 7 years ago

Hm.... I think being public does not necessarily mean we need to lock it down - we just need to make it explicit via the version number when we change the interface. I too don't think a lot of users will use it, but I feel this makes PerfCascade more flexible.

Alternatively we could break it into components and have a dedicated HAR-only repo that imports the renderer component, but I feel that adds unnecessary complexity.

micmro commented 7 years ago

@gitgrimbo Put the typings in a separate repo and have a PR for DefinitelyTyped, so (if accepted) it will be available as @type/har-format.

@tobli, I'm still thinking about the public/private of the waterfall, will not merge the PR until the types are on DefinitelyTyped, as I want to experiment with using this internally.

gitgrimbo commented 7 years ago

@micmro, great. Thanks!

micmro commented 7 years ago

@gitgrimbo the HAR typings got merged today are now available on npm as @types/har-format

gitgrimbo commented 7 years ago

Thanks @micmro. While the types weren't available as a @types I cloned this repo to a sibling folder of my projects and imported the types from there. All seemed to work ok.

I'll try the @types when I next get the chance.

micmro commented 7 years ago

@gitgrimbo not sure if it helps, but you can use it like this (via ES6 modules):

npm install --save @types/har-format
import { Har } from "har-format";

function getNonRedirectEntries(file: Har){
    return file.log.entries.filter(e => !e.response.redirectURL);
}
gitgrimbo commented 7 years ago

Swapped from local clone of har-format-ts-declaration repo to @types/har-format and all seems ok so far. Thanks.

micmro commented 7 years ago

fixed with v1.0.0