Skip to content

begin typescript conversion#5

Open
James-Burba wants to merge 12 commits intomasterfrom
typescript
Open

begin typescript conversion#5
James-Burba wants to merge 12 commits intomasterfrom
typescript

Conversation

@James-Burba
Copy link
Contributor

No description provided.

import nock from "nock";
import isEqual from "lodash.isequal";

interface Response {

Choose a reason for hiding this comment

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

If we want, we could use the types built in to express (express.Request and express.Response). If not, we can add a property to each [key: string]: any to allow any extra properties to be set

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Isn't that a little weird, since this library has nothing to do with express?

hmm what kind of extra properties would we want to set on these? They're there to serve a pretty specific purpose.

Choose a reason for hiding this comment

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

Just if the user wanted to add any additional properties to Request or Response, than the ones provided. Unless nobody would be providing anything else

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, no one should be doing that. maybe making these types at all is a mistake, I thought it would be easier but maybe not.

src/index.ts Outdated
}
}

export default function makeNockInspector(options: {

Choose a reason for hiding this comment

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

Instead of this, we can export the class declaration export default class NockInspector, then library consumers can just call new NockInspector

Copy link
Contributor Author

Choose a reason for hiding this comment

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

maybe... but don't we want this to work with the code we already have?

Choose a reason for hiding this comment

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

That's true, to match the current version we should have this same function exposed

private numberedResponses: { [key: number]: Response } = {};
private scope: nock;

constructor({

Choose a reason for hiding this comment

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

I think it would be nicer if we just added regular parameters instead of using an object parameter, since typescript gives us the definitions. Then we can declare it like:

constructor(
  basePath: string,
  endpoint: string,
  response: Response = { status: 200},
  method: string = 'get'
)

Choose a reason for hiding this comment

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

I think it's slightly more readable but either way works, really

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants