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

feat: add proper JSDoc annotations to all public api and generate TS … #33

Merged
merged 4 commits into from
Jul 15, 2020

Conversation

Hotell
Copy link
Contributor

@Hotell Hotell commented May 25, 2020

Closes #22

  • adds TS type generation on build which produces high quality types ( like axios )
  • all public API's are properly typed and use generics
  • adds strict type checking for implementation (with lowering the strictness level - as it's not that straightforward in vanilla codebase)

image

NOTES:

* @property {Options} config the request configuration
* @property {any} data the decoded response body
* @property {T} data the decoded response body
* @property {Headers} headers
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've added all fields that are assigned to the response to properly match implementation (dunno if it's on purpose, or we wanna strictly "match" axios response - in that case those need to be removed + omit those within implementation)

* @public
* @type {AbortController}
*/
redaxios.CancelToken = /** @type {any} */ (self).AbortController || Object;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is TypeScript dom types issue as AbortController is not defined on Window interface


/* Strict Type-Checking Options */
"strict": true, /* Enable all strict type-checking options. */
"suppressImplicitAnyIndexErrors": true,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

easy up strict checks

// "strictFunctionTypes": true, /* Enable strict checking of function types. */
// "strictBindCallApply": true, /* Enable strict 'bind', 'call', and 'apply' methods on functions. */
// "strictPropertyInitialization": true, /* Enable strict checking of property initialization in classes. */
"noImplicitThis": false, /* Raise error on 'this' expressions with an implied 'any' type. */
Copy link
Contributor Author

Choose a reason for hiding this comment

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

easy up strict checks

"forceConsistentCasingInFileNames": true /* Disallow inconsistently-cased references to the same file. */
},
"include": [
"src"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

enable on src/ only (excluding tests)

@@ -35,6 +36,7 @@
"karmatic": "^1.4.0",
"microbundle": "^0.11.0",
"sinon": "^8.0.4",
"typescript": "3.9.3",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this doesn't have to be as direct dep, microbundle will handle that in future

Copy link

@marvinhagemeister marvinhagemeister left a comment

Choose a reason for hiding this comment

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

This is fantastic! Love it 🙌

src/index.js Outdated Show resolved Hide resolved
*/

/** */
export default (function create(defaults) {
export default (function create(/** @type {Options} */ defaults) {
Copy link
Owner

Choose a reason for hiding this comment

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

If it's easier to break the export and declaration out, I think microbundle+terser will collapse them all the same:

/** @param {Options} defaults */
function create() {
  // ...
}
export default create();

@Shuunen
Copy link

Shuunen commented Jun 17, 2020

"Compressed Size / build (pull_request)" fails, why ? Looking at "details" didn't helped me understand the issue here, this MR increased the pkg size too much ?

@marvinhagemeister
Copy link

The relevant error message is:

 src/index.js(210,25): error TS2339: Property 'withCredentials' does not exist on type 'Options'.

Looks like there is a type error in the changes

@developit developit merged commit 8ed737f into developit:master Jul 15, 2020
@developit developit mentioned this pull request Jul 15, 2020
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.

Add typescript types
4 participants