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

chore: Make "params" argument optional in getter methods #836

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

ryami333
Copy link
Contributor

@ryami333 ryami333 commented Jul 8, 2024

Small (backward-compatible) change to the .getStory (et al) methods to make the second argument (params) optional. So that you can simplify your code in the following way:

import StoryblokClient from "storyblok-js-client";

const storyblokClient = new StoryblokClient({
  // …
});

-storyblokClient.getStory(slug, {}) // Passing redundant empty-object 
+storyblokClient.getStory(slug)
  .then(console.log)
  .catch(console.error);
  • Bugfix
  • Feature
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes, no api changes)
  • Other (please describe):

How to test this PR

  • Make a call to client.getAll, client.get, client.getStories or client.getStory.
  • Don't pass the params argument.
  • Expect no Typescript error.
  • Expect no runtime error.

What is the new behavior?

There is no Typescript or runtime error when you defer to the client defaults (omit the params argument) when calling the Storyblok client's getter methods.

@alvarosabu alvarosabu changed the title Chore: Make "params" argument optional in getter methods chore: Make "params" argument optional in getter methods Aug 23, 2024
@alvarosabu alvarosabu added the p2-nice-to-have [Priority] Lower priority, beneficial enhancements that are not urgent. label Aug 23, 2024
@alvarosabu alvarosabu self-assigned this Aug 23, 2024
@alvarosabu alvarosabu self-requested a review August 23, 2024 08:46
@alvarosabu
Copy link
Contributor

alvarosabu commented Aug 23, 2024

Hi @ryami333 thanks a lot for this improvement, I'm good with it being merge, let me check if we need to change documentation before the final ok.

I will merge #550 soon, maybe we will need to pull the changes in this PR.

I keep you posted. Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p2-nice-to-have [Priority] Lower priority, beneficial enhancements that are not urgent.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants