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

Use symbols to allow simpler return objects #34

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

Use symbols to allow simpler return objects #34

southpolesteve opened this issue Jul 9, 2018 · 6 comments
Labels
breaking non-additive functional behavior changes discussion-wanted Need a discussion on an area
Projects

Comments

@southpolesteve
Copy link
Contributor

southpolesteve commented Jul 9, 2018

A couple user feedback sessions have shown that returning response wrapper objects from function calls is a bit confusing. Most users (especially new ones) will not need access to raw response headers or other metadata. However, advanced users do which is why we return these objects. I think we can support both users needs by storing response metadata using Symbols. These will allow us to safely store metadata on an object without conflicting with user keys.

// ====  Current ====
function createItem(body: any, options?: RequestOptions): Promise<Response<T>>;
function updateItem(body: any, options?: RequestOptions): Promise<Response<T>>;

const { result: item, headers } = createItem({})
item.name = 'foo'
updateItem(item)

// ====  Using Symbols ====
// Probably setup once and exported from the client
const headersSymbol = Symbol('CosmosDBHeaders');

interface Response {
    [headersSymbol]: RequestHeaders;
};

function createItem(body: any, options?: RequestOptions): Promise<T & Response>;
function updateItem(body: any, options?: RequestOptions): Promise<T & Response>;

const item = createItem({})
const headers = item[headersSymbol]
// OR
const { ...item, [sym]: headers } = createItem({})
// OR Can also be encapsulated on a client method if we don't want to expose Symbol magic
const headers  = client.getHeaders(item)

item.name = 'foo'
updateItem(item)
@southpolesteve southpolesteve added breaking non-additive functional behavior changes discussion-wanted Need a discussion on an area labels Jul 9, 2018
@southpolesteve southpolesteve changed the title Use symbols to allow simpler objects Use symbols to allow simpler return objects Jul 9, 2018
@christopheranderson
Copy link
Contributor

Some open questions for me:

  • Symbols don't work in IE at all. How would we support IE?
  • Symbols are included in JSON.stringify() in Edge, but are otherwise ignored. Do we just provide that note to customers that they need to use a helper method to filter them from JSON.stringify() if they are using that in the browser?

@southpolesteve
Copy link
Contributor Author

Surfacing some chat discussions

  • There is a polyfill for Symbol we can document for IE users
  • Edge appears to have fixed this bug

@christopheranderson christopheranderson added this to Needs Discussion in Kanban Jul 9, 2018
@christopheranderson
Copy link
Contributor

Have you tried out the intellisense for T & Response? Does it work?

@christopheranderson
Copy link
Contributor

Kiran brought up a valid point that we want users to be recording their activity ids in their telemetry, etc., so it's worth considering if this is actually going to save any lines of code or complexity over object deconstruction.

Today:

const { result, headers } = await items.readAll();
console.log(headers["x-ms-activityid"]);

With proposed change:

const result = await items.readAll();
console.log(client.getHeaders(result)["x-ms-activityid"]);

There is a lot of magic involved here. I think we should do some user testing with this after we've figured out #39. I'm worried about this for the same reasons I'm worried about using Proxies to overload indexing.

@southpolesteve
Copy link
Contributor Author

Intellisense works. It actually shows the symbol key by whatever var name it was assigned to. Even if in another file.
screenshot 2018-07-10 09 23 39

Fair point above. I do wanna see how this tests. To me, the advantage comes when you don't want to extract the headers. It makes that case much simpler while having a similar experience for the advanced use case.

Worth noting that React does something in this vein with symbols as values and providing a helper function https://github.com/facebook/react/blob/fc3777b1fe295fd2661f1974f5587d214791f04b/packages/react-is/src/ReactIs.js#L25

@southpolesteve
Copy link
Contributor Author

Per comment in #53 decided to hold off on this for now.

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
breaking non-additive functional behavior changes discussion-wanted Need a discussion on an area
Projects
No open projects
Kanban
  
Done
Development

No branches or pull requests

2 participants