-
Notifications
You must be signed in to change notification settings - Fork 0
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
base: master
Are you sure you want to change the base?
Conversation
import nock from "nock"; | ||
import isEqual from "lodash.isequal"; | ||
|
||
interface Response { |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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: { |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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({ |
There was a problem hiding this comment.
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'
)
There was a problem hiding this comment.
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
No description provided.