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

Turn most types into interfaces #615

Merged

Conversation

denis-rossati
Copy link
Contributor

Summary

This PR changes most types to be interfaces, thus leveraging the extensibility of the library.


Yet, there some types left in the code:

I couldn't manage to turn callbacks or primitives into interfaces:

I haven't found a way to create an equivalent interface to these types, mostly because you can't extend an Excludes<>, Partial<> or a set of union types:

Some of them I thought to be better as a type, but please let me know if I'm wrong

*The value in these cases are all interfaces, so I guess it's fine

**Even if I turned these into interfaces, the ESLint claimed that I should not extend only one interface without any adition, since it will be some sort of copy of the extended interface


Closes #606

@denis-rossati
Copy link
Contributor Author

@arthurfiorette can you provide an example of how can I help regarding "Also, clarify how to extend axios-cache-interceptor types documentation"? For me it's not very clear how the lib deals with this communication

@codecov
Copy link

codecov bot commented Jul 23, 2023

Codecov Report

Merging #615 (f6e8dff) into main (998e96c) will not change coverage.
Report is 7 commits behind head on main.
The diff coverage is n/a.

❗ Current head f6e8dff differs from pull request most recent head de28c0c. Consider uploading reports for the commit de28c0c to get more accurate results

@@            Coverage Diff            @@
##              main      #615   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           17        17           
  Lines          578       578           
  Branches       190       190           
=========================================
  Hits           578       578           
Files Changed Coverage Δ
src/cache/create.ts 100.00% <ø> (ø)
src/interceptors/util.ts 100.00% <ø> (ø)
src/storage/build.ts 100.00% <ø> (ø)
src/storage/memory.ts 100.00% <ø> (ø)

@arthurfiorette
Copy link
Owner

Thanks for your work!

You forgot this one:

export type CacheAxiosResponse<R = any, D = any> = AxiosResponse<R, D> & {

@arthurfiorette
Copy link
Owner

@arthurfiorette can you provide an example of how can I help regarding "Also, clarify how to extend axios-cache-interceptor types documentation"? For me it's not very clear how the lib deals with this communication

As this package overrides the AxiosInstance interaface with its own types, in case of adding other interceptors or custom code which requires its own properties, its needed to do the following:

// (I typed this out of my cellphone, so IDK its 100% right)
declare module 'axios-cache-interceptor' {
  interface CacheRequestConfig<R = any, D = any> {
    myProperty: number;
  }
}

If you could just add an section on how to do the above in case you need to further customize your axios instance in general...

@arthurfiorette
Copy link
Owner

Também queria dizer que, como ex morador de pedra azul, morar na serra é loucura hahahahah

@denis-rossati
Copy link
Contributor Author

Thanks for your work!

You forgot this one:

export type CacheAxiosResponse<R = any, D = any> = AxiosResponse<R, D> & {

Yep, I really forgot 😅

Upon changing locally, I noticed that would require me to add headers: AxiosRequestHeaders; to the CacheRequestConfig, like this:

export type CacheRequestConfig<R = any, D = any> = AxiosRequestConfig<D> & {
  id?: string;

  cache?: false | Partial<CacheProperties<R, D>>;

  headers: AxiosRequestHeaders; // <---- adding this property 
};

Which will also require changing some unit tests (which is weird, because axios claims this property as optional, I guess I'm missing something) is that alright?

@denis-rossati
Copy link
Contributor Author

Também queria dizer que, como ex morador de pedra azul, morar na serra é loucura hahahahah

KKKKKKKKKKKKKKKK é outro nível mesmo

@arthurfiorette
Copy link
Owner

Which will also require changing some unit tests (which is weird, because axios claims this property as optional, I guess I'm missing something) is that alright?

Maybe you need to extend InternalCacheRequestConfig? Axios v1 is experimenting with a custom header typing / object which is breaking everywhere... So I guess we'll have to play a little with this until we get it right. Just take care to not break/force anything... this should be a non breaking change for axios-cache-interceptor users

Copy link
Owner

@arthurfiorette arthurfiorette left a comment

Choose a reason for hiding this comment

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

Sorry for the delay, As soon as this is changed, i'll merge this PR. Thanks a lot!

docs/src/guide/interceptors.md Outdated Show resolved Hide resolved
docs/src/guide/interceptors.md Show resolved Hide resolved
@denis-rossati
Copy link
Contributor Author

Don't worry about the delay. Thanks for the help, in the next few days when I have less work to do I want to start working on other issues :)

Also, the tests at the CI are failing but locally they're passing 🤔 I don't know how but it's possible that I broke them when updating the docs?

Copy link
Owner

@arthurfiorette arthurfiorette left a comment

Choose a reason for hiding this comment

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

lgtm

@arthurfiorette
Copy link
Owner

Thanks @denis-rossati. Thanks for your work, probably the CI failed because you merged from main while it was broken :)

@arthurfiorette arthurfiorette merged commit 0199ad6 into arthurfiorette:main Jul 30, 2023
3 of 4 checks passed
arthurfiorette added a commit that referenced this pull request Jul 30, 2023
…and other objects (#616)

* refactor: change id generation location

* fix: lint

* refactor: ensures id will be created only if cache is not false

* chore(deps-dev): bump vitepress from 1.0.0-beta.5 to 1.0.0-beta.6 (#618)

* chore(deps-dev): bump @types/node from 18.16.19 to 18.17.0 (#617)

* chore(deps-dev): bump tslib from 2.6.0 to 2.6.1 (#619)

* chore(deps-dev): bump @types/node from 18.17.0 to 18.17.1 (#620)

* chore: dependabot

* ci: dependabot

* feat: handle non axios errors rejections (#609)

* fix: correct config re throw

* chore(deps-dev): bump eslint-config-prettier from 8.8.0 to 8.9.0 (#624)

* chore(deps-dev): bump jest from 29.6.1 to 29.6.2 (#622)

* feat: turn most types into interfaces (#615)

* Turn most types into interfaces

* Turn 'CacheAxiosResponse' into a interface

* Update docs

* Change docs to be more didactic

* chore(deps-dev): bump jest-environment-jsdom from 29.6.1 to 29.6.2 (#623)

* chore: removed unused eslint comment

* feat: handle errors on object-code

* feat: bring back ids

* chore: lint

---------

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Arthur Fiorette <arthur.fiorette@gmail.com>
Co-authored-by: Denis Rossati <denis.rossatiramos@gmail.com>
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.

Change all types to interfaces
2 participants