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

.get is confusing #39

Closed
christopheranderson opened this issue Jul 9, 2018 · 6 comments
Closed

.get is confusing #39

christopheranderson opened this issue Jul 9, 2018 · 6 comments
Assignees
Labels
discussion-wanted Need a discussion on an area
Projects

Comments

@christopheranderson
Copy link
Contributor

We've gotten some feedback from user studies that .get is confusing.

Let's track in this issue our ideas

@christopheranderson
Copy link
Contributor Author

Couple of ideas:

  1. Get rid of the "singular" objects (aka database vs databases). Move all the singular object methods to the plural.
  • Example: const db = client.databases.get(id); await db.delete(); -> await client.databses.delete(id);
  • Pros: Less objects to navigate
  • Cons: you still need to have the builder pattern addressed somewhere, so this idea is incomplete. You can no longer pass around a object with a reference to your path for things like read/replace, which makes the DI stuff we were talking about not work. It also differs from C# really heavily, so we'd need sign off from Kirill on this.
  • Overall, I'm not a fan of this approach. It could be used in combination of some other approaches though.

2a. Rename {Resource}s.get(id: string) => <{Resource}> to {Resource}s.{resource}(id: string} => <{Resource}>

  • Example: const db = client.databases.get(id); -> const db = client.databases.database(id);
  • Pros: no verb in the name makes it less easy to confuse with read. Obvious which resource you'll get
  • Cons: more verbose. Less consistent (can't do an interface that each plural object has a get method/etc.)

2b. Rename get to something less verby like "reference", "ref", "for"

  • Example: const db = client.databases.get(id) -> const db = client.databases.ref(id)
  • Pros: Consistency is retained, but no longer sounds like a verb
  • Cons: Not obvious what it does, especially compared to 2a.
  1. Proxy overload of index to get
  • Example: const db = client.databases[id]
  • Pros: Less verbose. Avoids making anything that looks like an async call.
  • Cons: Proxies are weird to some folks (potentially, haven't measured this). Could be less intuitive or create side effect confusion. Doesn't necessarily get rid of get -> read confusion unless we also rename get.
  1. Return reference object in response from async operations.
  • Example: const { resource: db} = client.databases.create({id: foo});
  • Pros: Helps reduce number of lines of code for many scenarios (like db and container create)
  • Cons: Doesn't remove need for .get because we don't want to force an async operation just to get a reference. Won't work with Steve's idea for simplifying the response types.

I'm actually in favor of combining a few options. As long as one option isn't any more performant than another so we have to tell folks "well, you should do A instead of B, even though we let you do B".

Thinking through what I expect folks to do, I'd expect them to mostly use 4 simply because a lot of folks have start up code which is like "createIfNotExists" for db and container, so they'll already have async operations we can return a reference object from.

I think 2b is a good solution to avoiding get -> read confusion, so I'd combine that with 4.

I still like the idea of doing 3, but its less necessary, so we can weigh the risk or do it in a separate PR.

I think 1 pairs nicely with 4, but 2b + 4 would be enough to test the idea, so 1 could also be post-poned.

@christopheranderson christopheranderson added the discussion-wanted Need a discussion on an area label Jul 9, 2018
@southpolesteve
Copy link
Contributor

I think 4 and my Symbol proposal are compatible. We can store the header data on the reference object. Or return an intersection type (Database & Response).

@southpolesteve
Copy link
Contributor

3 could be done without proxies by moving to singular methods for reference objects:

Proxy: const db = client.databases[id]
Singular: const db = client.database(id)

In the latter case, what if we dropped get entirely?

@christopheranderson
Copy link
Contributor Author

christopheranderson commented Jul 10, 2018

Yeah, I think we can make 4 compatible, but it does increase complexity slightly, so I figured I'd call that out as a con.

I think your suggestion on how to do 3 without proxies is just 2a with a change to move/duplicate the singular accessor to the parent object type? Let's just call this 5 instead of 2a`

Comparing 2a and 5:

  • 2a:
const {result: items} = await client.databases.database(id).containers.container(id).items.readAll();
const {result: item} = await client.databases.database(id).containers.container(id).items.item(id).read();
  • 2a` 5
const {result: items} = await client.database(id).container(id).items.readAll();
const {result: item} = await client.database(id).container(id).item(id).read();

And you'd just drop get all together, so 3 and 2b wouldn't be necessary?

  • Pros: less code than 2a/2b. Still intuitive compared to 2b.
  • Cons: Not consistent method names like 2a.
  • Not really a con, but note worthy - splits our call tree into two: plural bag of methods and singular builder method

You could still do 4 & 5 together.

You could still do 1 & 5 together, but it's less of an improvement than with 2a/2b + 1.

Overall, I think I like it. While I think the Proxies version would be better overall, I think this is likely more intuitive for the current state of JavaScript. Maybe in v3, we could move to proxies without any concerns on user experience.

@christopheranderson
Copy link
Contributor Author

christopheranderson commented Jul 10, 2018

Held a meeting to discuss.

Notes:

4 & 5 make sense to do. Chris is gonna pick up 4 and Steve is gonna pick up 5. We'll prioritize getting database, container, and item done by tomorrow morning so Deborah can update the sample for Thursday user study.

Actions:

@christopheranderson
Copy link
Contributor Author

This has now been addressed in the 2.0.0-3 release.

Kanban automation moved this from Needs Discussion to Done Jul 31, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion-wanted Need a discussion on an area
Projects
No open projects
Kanban
  
Done
Development

No branches or pull requests

3 participants