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

begin typescript conversion #5

Open
wants to merge 12 commits into
base: master
Choose a base branch
from
Open

begin typescript conversion #5

wants to merge 12 commits into from

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