Skip to content

Conversation

@rerinho
Copy link
Contributor

@rerinho rerinho commented Jan 13, 2022

Hello!

Changes:

  1. Added memoize option in the Request type (its true by default for GET requests)
  2. Added request memoizable validation before the response is set in memoisedResults.
  3. Added 3 tests
    a. When no memoize option is informed (only for non-GET requests)
    b. When memoize option is false (should not add the response in the memoizedResults)
    c. When memoize option is true (current version default approach)

Motivation:

This change was necessary for a specific use case presented bellow:

1. Request a resource from another API.
2. Update some property of this resource.
3. Request the same resource again.

In this case, the resource received in the step 3 will be the same as in step 1, as the memoized is returned.

The memoize option allows to disabling storage in memoizedResults for specific requests.

Copy link

@contesini contesini left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link

@joaoantoniosouza joaoantoniosouza left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@JustinTRoss JustinTRoss left a comment

Choose a reason for hiding this comment

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

Good work! 💪 This feature is dearly needed.

@JustinTRoss
Copy link
Contributor

Is there anywhere we can find a list of maintainers who can approve the running of workflows? @StarpTech

@StarpTech
Copy link
Owner

Hi @JustinTRoss, I'll try to review it next week. Thanks for the work.

@JustinTRoss
Copy link
Contributor

Thanks @StarpTech 🙏 . @rerinho is the hard-worker 🔨 . Just looking for the lever to advance this to the next step 🙃 .

Copy link
Owner

@StarpTech StarpTech left a comment

Choose a reason for hiding this comment

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

LGTM except for a small thing. Great PR.

Copy link
Owner

@StarpTech StarpTech left a comment

Choose a reason for hiding this comment

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

LGTM

@StarpTech StarpTech merged commit cb6d72e into StarpTech:main Feb 7, 2022
@StarpTech
Copy link
Owner

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.

6 participants