Types #17
Types #17
Conversation
| ) | ||
|
|
||
| const expect = module.exports = actual => { | ||
| const chainable = function (x) { return this.call(x) }; delete chainable.length |
There was a problem hiding this comment.
Rewrite
This caused
The operand of a 'delete' operator cannot be a read-only property.ts(2704)
And it does not seem to have any effect anyway:
lib/axios.js
Outdated
| axios.defaults.baseURL = url | ||
| axios.default.defaults.baseURL = url | ||
| }}) | ||
| return super.axios = axios |
There was a problem hiding this comment.
Rewrite
to my understanding, this super.… = … technique was used to have a truly private variable. I replaced them with actual private variables.
| * @param {(message?: any, ...optionalParams: any[]) => void} capture | ||
| */ | ||
| log (_capture) { | ||
| log (capture) { |
There was a problem hiding this comment.
Renamed, as underscore is generally used for "variables I don't use", "don't care", or "throwaway" (both, in Python, as well as in the ESLint rules no-unused-vars).
If there's a good reason for keeping the underscore, do let me know.
| */ | ||
| return (msg,fn) => Promise.all (table.map (each => { | ||
| const args = Array.isArray(each) ? each : [each], [label] = args | ||
| return this (format(msg, label), ()=> fn(...args)) |
There was a problem hiding this comment.
not entirely sure what is supposed to happen here. this refers to module, which is not callable.
I guess this is supposed to be return each (...)? (If we intend to fix this, be aware that each (the outer function) is shadowed by each (the parameter of the inner function), so some renaming or aliasing needs to take place. Or use this.exports(...).
| return require.cache[path] = o?.virtual ? fn() : Object.assign (require(path), fn()) | ||
| }} | ||
| } | ||
| return undefined |
There was a problem hiding this comment.
this happened implicitly before and TS didn't like it that way.
| get eq() { return this.equals } | ||
| get eql() { return this.eqls } | ||
| // @ts-expect-error - FIXME! This is probably actually missing! | ||
| get exists() { return this.defined } |
There was a problem hiding this comment.
I can not find defined in the class hierarchy and TS is also complaining about it missing. Unless this is injected in some voodoo way (please don't...) this is probably actually missing and should be amended.
| if (is.array(a)) return a.includes(x) || this._deep && a.some(o => compare(o,x)) | ||
| if (is.set(a)) return a.has(x) | ||
| if (is.object(a)) return compare(a,x) | ||
| return false |
There was a problem hiding this comment.
this turns the return type from boolean | undefined into boolean. I assume that we used the implicit undefined as falsy value, not to have any explicit === undefined comparison. If we did rely on this ternary return type, let me know.
| get true() { return this.assert(a => a === true) || this.should`be ${true}` } | ||
| get false() { return this.assert(a => a === false) || this.should`be ${false}` } | ||
| get empty() { return this.assert(a => !a?.length === 0 || Object.keys(a).length === 0) || this.should`be empty` } | ||
| // @ts-expect-error - FIXME: this is always false due to precedence! |
There was a problem hiding this comment.
!a?.length is coerced into boolean, followed in a strict comparison between boolean and 0, which always yields false.
| function afterEach(): void; | ||
| function afterAll(method: Function): void; | ||
| function afterAll(message: string & Function?, method: Function): void; | ||
| function expect(_:any): void; |
There was a problem hiding this comment.
what is the signature for this parameter? I derived it from node-test.js where it is used as expect(this), but no idea what the proper type should be there.
| */ | ||
| function (url, config) { | ||
| if (new.target) return new Naxios (url) | ||
| else config = typeof url === 'object' ? url : { url, ...config } |
There was a problem hiding this comment.
url appears to be an object only in some cases. I assume in other cases it can be a string (I therefore added the above type). The constructor call would then attempt to destructure a string. While that "works":
> var x = {...'str'}
undefined
> x
{ '0': 's', '1': 't', '2': 'r' }I guess that is not really intended behaviour?
|
@chgeo as discussed, I'll temporarily leave the types as is. There are still various errors to address, but I think it makes sense to resolve the open discussion points (see my comments on this PR and |
Adds type definitions.
Also, some rewrites were required. I tried to rewrite as sparingly as possible. See comments. If you see any rewrite that changes the behaviour in an unacceptable way, please let me know.