Skip to content
This repository has been archived by the owner on Dec 5, 2022. It is now read-only.

Add typings #226

Merged
merged 4 commits into from Apr 23, 2018
Merged

Add typings #226

merged 4 commits into from Apr 23, 2018

Conversation

arhont375
Copy link
Contributor

HI,

Thank you for this great project.
This PR is adds TypeScript typings for this package, that will help other people to get information about possible options from IDE suggestions. A bit less possible to make mistake.
And also will allow using of this library in TypeSript projects without additional works.

@ruiaraujo
Copy link
Contributor

@eugenkiss Wanna help here?

@eugenkiss
Copy link

I believe @vigneshshanmugam should check if the shape of the Tailor declaration correctly corresponds to the JavaScript one.

One remark for @arhont375: In the example folder it'd be nice to have a README with information on how to run the example (for example using ts-node) since node cannot run TypeScript files directly of course.

@codecov
Copy link

codecov bot commented Apr 17, 2018

Codecov Report

Merging #226 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #226   +/-   ##
=====================================
  Coverage      99%    99%           
=====================================
  Files          15     15           
  Lines         603    603           
  Branches      112    112           
=====================================
  Hits          597    597           
  Misses          6      6

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0d930d3...d0e0ca6. Read the comment docs.

@arhont375
Copy link
Contributor Author

@eugenkiss
Thank you for your suggestion! Updated this PR.
Seems build is failing, but as I understand it should not be related to my changes - I haven't changed any Tailor code.

Install TypeScript compiler and execute following;

```bash
tsk ./index.ts
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tsc?

index.d.ts Outdated
amdLoaderUrl?: string
, fetchContext?: (req: IncomingMessage) => Promise<object>
, fetchTemplate?: (templatesPath: string, baseTemplateFn: (path: string) => string) => Promise<any>
, filterRequestHeaders?: (attributes: object, req: IncomingMessage) => object
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we can type the fragmentAttributes since we expose only few of them by default.

index.d.ts Outdated
, maxAssetLinks?: number
, pipeAttributes?: (attributes: object) => object
, pipeInstanceName?: string
, requestFragment?: (filterHeaders: (attributes: object, req: IncomingMessage) => object, url: Url, attributes: object, req: IncomingMessage) => Promise<IncomingMessage>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

returns Promise<ServerResponse>

index.d.ts Outdated
, fetchContext?: (req: IncomingMessage) => Promise<object>
, fetchTemplate?: (templatesPath: string, baseTemplateFn: (path: string) => string) => Promise<any>
, filterRequestHeaders?: (attributes: object, req: IncomingMessage) => object
, filterResponseHeaders?: (attributes: object, req: ServerResponse) => object
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo: req -> res

@vigneshshanmugam
Copy link
Collaborator

👍

2 similar comments
@DeTeam
Copy link
Contributor

DeTeam commented Apr 23, 2018

👍

@eugenkiss
Copy link

👍

@vigneshshanmugam vigneshshanmugam merged commit a12404d into zalando:master Apr 23, 2018
@vigneshshanmugam
Copy link
Collaborator

@arhont375 Thanks for the PR 🙂

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

Successfully merging this pull request may close these issues.

None yet

5 participants