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

Tweek's javascript client fetch function should have a simpler option when there's no need to override context #100

Open
Gabrn opened this issue Oct 23, 2018 · 9 comments

Comments

@Gabrn
Copy link
Contributor

Gabrn commented Oct 23, 2018

The api for fetching configurations forces the client to pass a configuration object with a nested context object and another nested object per entity type even if there's no need to override the context and there's a need for only one entity. It is also a little weird for the client in general when the context is part of the options object.

Suggestion:

Create an overload utility function for passing only entity type and id:

await tweekClient.fetch("path/to/configuration", "user", "Yshay", options);
@Gabrn Gabrn changed the title Tweek's javascript client fetch function should be simpler when there's no need to override context Tweek's javascript client fetch function should have a simpler option when there's no need to override context Oct 23, 2018
@Yshayy
Copy link
Contributor

Yshayy commented Oct 23, 2018

What about { user: “John”}, it’s more consistent with both js and Tweek rest style?

@Gabrn
Copy link
Contributor Author

Gabrn commented Oct 23, 2018

Ok, lets do that then.
I also think it's a good idea to separate the context object from the other options object.
I don't think people who use the js sdk think about how it looks like in rest.

So it should be:

  1. client.fetch("config", {user: "John"}, options)
  2. client.fetch("config", {user: {id: john, contextOverride: "bla"}}, options)

@Yshayy
Copy link
Contributor

Yshayy commented Oct 23, 2018

It’s not necessary override, it’s context properties

@Yshayy
Copy link
Contributor

Yshayy commented Oct 23, 2018

the semantics of context are important whether you use rest or other tweek client

@Yshayy
Copy link
Contributor

Yshayy commented Oct 23, 2018

Also the client support giving context in ctor.

@tal952
Copy link

tal952 commented Oct 23, 2018

Also the client support giving context in ctor.

Wow, you right!
So the context in fetch basically used only for context override?

@Yshayy
Copy link
Contributor

Yshayy commented Oct 23, 2018

Could be expansion, not just override.
I think the context shorthand syntax is useful and more concise.
If we use it, it should be implemented in ctor and Tweek-local-cache lib as well

@Gabrn
Copy link
Contributor Author

Gabrn commented Oct 23, 2018

Cool, so just so we’re clear which options do you think we should implement @Yshayy

@Yshayy
Copy link
Contributor

Yshayy commented Oct 23, 2018

I think adding Shorthand syntax support for context.
Support of string identifier instead of an object with id field (full object should be still supported)

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

No branches or pull requests

3 participants