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

Improve readme #6

Closed
wants to merge 1 commit into from
Closed

Conversation

teohhanhui
Copy link

  • Rename driver from Fetch to HTTP
  • Add example with input and init

- Rename driver from `Fetch` to `HTTP`
- Add example with `input` and `init`
@teohhanhui
Copy link
Author

@secobarbital Since you've made the changes for the first item, I'll open a separate PR once you've merged in your commit.

@teohhanhui
Copy link
Author

Also, I want to provide a reference to the documentation on MDN, but I'm concerned that we don't support input as string. I think we should change that.

@secobarbital
Copy link
Collaborator

We do support input as string, actually.

@teohhanhui
Copy link
Author

src/index.js#L8
and
src/index.js#L22
do not consider the possibility of request.input being string.

@secobarbital
Copy link
Collaborator

src/index.js#L4 does, and normalizes it into the object structure so that downstream we don't have to switch.

@teohhanhui
Copy link
Author

No it doesn't. That only handles the case of request (what's emitted to the driver) itself being a string. It doesn't handle cases where request.input is string (one would expect that to be acceptable if we were to follow the spec).

@teohhanhui
Copy link
Author

And we should probably rename input to request in normalizeRequest to avoid confusion.

@secobarbital
Copy link
Collaborator

Agree on renaming input to request in normalizeRequest but not the object format. It is an imperfect representation of a list of arguments to fetch so I took the liberty of breaking the overloaded first argument into separate url and input keys.

If we took an array instead I would agree, but then we would need a way to attach the key. Another way I had considered was to call it request instead of input since it is a Request object, but request.request gets kinda recursive. I had the same issue with route in the routing libraries too.

I guess that's why naming variables is one of the two hard problems in CS ;)

@teohhanhui
Copy link
Author

It is an imperfect representation of a list of arguments to fetch so I took the liberty of breaking the overloaded first argument into separate url and input keys.

What about fetchSpec with url, request (because it really is a Request object) and options keys?

If we took an array instead I would agree, but then we would need a way to attach the key.

That's what I tried to do just now, and of course I ran into the same issue... 😛

@secobarbital
Copy link
Collaborator

Feel free to do a PR so there's no doubt about what you mean. Now I actually kinda like the args array idea and may create a PR to discuss with you. And I haven't forgotten about the link to the spec and the examples of using Request object. Maybe you can reopen this one for those?

@teohhanhui teohhanhui deleted the improve-readme branch February 3, 2016 07:27
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.

None yet

2 participants