Skip to content
This repository has been archived by the owner on Aug 24, 2021. It is now read-only.

update eyre documentation #1089

Merged
merged 44 commits into from
Jul 21, 2021
Merged

update eyre documentation #1089

merged 44 commits into from
Jul 21, 2021

Conversation

tinnus-napbus
Copy link
Contributor

removes old documentation and adds a scry ref, data types ref, external
& internal api ref, and examples.

@drbeefsupreme

removes old documentation and adds a scry ref, data types ref, external
& internal api ref, and examples.
@tinnus-napbus
Copy link
Contributor Author

note this requires the change to faq.md in urbit/urbit.org#642 in order for zola to build it successfully

Copy link
Member

@Fang- Fang- left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow! Another big-boy PR right here. Very thorough, complete and clear documentation. Thank you for this!

The only thing I'm really missing here is some description of the header handling that Eyre does for you. CORS headers come up implicitly, but the behavior isn't talked about in too much detail (and extends to OPTIONS requests wholesale). Eyre respects the Forwarded header when figuring out the address for inbound requests (but only if the request came from localhost). Additionally, Eyre (and/or the runtime?) take care of the content-length header for you.

I also left some corrections, and a bunch of suggestions and style nits in the comments below. Hope they're useful, and let me know if you want me to further clarify on anything here. (:

arvo/eyre/data-types.md Outdated Show resolved Hide resolved
arvo/eyre/data-types.md Outdated Show resolved Hide resolved
arvo/eyre/data-types.md Outdated Show resolved Hide resolved
arvo/eyre/data-types.md Outdated Show resolved Hide resolved
arvo/eyre/data-types.md Outdated Show resolved Hide resolved
arvo/eyre/examples.md Outdated Show resolved Hide resolved
arvo/eyre/examples.md Outdated Show resolved Hide resolved
arvo/eyre/examples.md Outdated Show resolved Hide resolved
arvo/eyre/examples.md Show resolved Hide resolved
arvo/eyre/examples.md Show resolved Hide resolved
@basilesportif
Copy link
Contributor

@tinnus-napbus fang had some changes requested here, wanted me to re-ping you

tinnus-napbus and others added 17 commits June 23, 2021 17:37
per recommendation

Co-authored-by: fang <github@fang.io>
per recommendation

Co-authored-by: fang <github@fang.io>
per recommendation

Co-authored-by: fang <github@fang.io>
per recommendation

Co-authored-by: fang <github@fang.io>
per recommendation. Fixes to imply you're implicitly acking all previous events

Co-authored-by: fang <github@fang.io>
per recommendation

Co-authored-by: fang <github@fang.io>
per recommendation

Co-authored-by: fang <github@fang.io>
per recommendation

Co-authored-by: fang <github@fang.io>
per recommendation

Co-authored-by: fang <github@fang.io>
per recommendation

Co-authored-by: fang <github@fang.io>
per recommendation

Co-authored-by: fang <github@fang.io>
- various phrasing fixes
- change 'kick' to quit
- explain all sse events are acked, not just facts
- restructure tasks so args aren't separate to task tag & merge
  "accepts" into description
- clarifications about agents being bound to url paths
- add note about eyre making a best effort convert facts to json
- other misc changes I can't remember
- remove x from beginning of scry paths
- make explanation about @ta-encoded @t clearer and accurate
- truncate long example
- improve description of the channel system
- improve description of the scry interface
- correct information regarding acking events
- correct information regarding sequentiality (or not) of IDs
- fix content-length formatting functions
- restructure with separate authentication section, remove deplicate
  authentication instructions and instead link to that section
- maybe other misc fixes
- add note about /lib/server/hoon
- rephrase to imply that EventSource isn't the only javascript SSE handler
Copy link
Member

@Fang- Fang- left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All good changes so far, thank you!

arvo/eyre/data-types.md Outdated Show resolved Hide resolved
arvo/eyre/external-api-ref.md Outdated Show resolved Hide resolved
@tinnus-napbus
Copy link
Contributor Author

ok I've addressed most of the comments. The original point about covering Eyre's CORS capabilities I've DM'd @Fang- to discuss what is necessary.

On the point about spider, yeah it's technically true that the spider interface isn't directly a function of Eyre. It is kinda a de facto thing though, it's automatically bound and threads are kinda fundamental despite spider not being a vane itself. My feeling is that practically it's useful to have all the documentation of web interfaces in one place and the previous "using eyre" documentation covered threads. I probably could find a place for it in the threads documentation though, idk. What do you think @drbeefsupreme ?

On the point about the explanations of Eyre's behaviour wrt generators bound with %serve and agents bound with %connect, maybe it's not ideal to have that info on the tasks page but idk it's useful and I'm not sure where else to put it. I think putting it all in the examples page isn't the best, nor on overview because it's too technical. I could create another document that covers the %serve and %connect cases in more detail but then I'm not sure there's enough info to warrant a separate page. What do you think @drbeefsupreme ?

@tinnus-napbus
Copy link
Contributor Author

also sorry for not addressing this stuff earlier, I missed github's email about it and assumed nobody had got around to reviewing stuff

arvo/eyre/examples.md Outdated Show resolved Hide resolved
Copy link
Member

@Fang- Fang- left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for all your work and changes here! This looks good to go, once @drbeefsupreme has settled the remaining discussion here and given his blessing.

On the point about spider, yeah it's technically true that the spider interface isn't directly a function of Eyre. It is kinda a de facto thing though, it's automatically bound

Counterpoint: it's only automatically bound if the spider app is started, and many apps bind endpoints when they start, and many apps are started by default. We're not talking about file-server here either, even though that's arguably even more fundamental (functionality-wise) than spider.

Copy link
Contributor

@drbeefsupreme drbeefsupreme left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pretty much all very minor grammar nitpicks. One of the task entries was also incomplete. Other than that, LGTM!

arvo/eyre/_index.md Outdated Show resolved Hide resolved
arvo/eyre/data-types.md Outdated Show resolved Hide resolved
arvo/eyre/examples.md Outdated Show resolved Hide resolved
arvo/eyre/examples.md Outdated Show resolved Hide resolved
arvo/eyre/data-types.md Outdated Show resolved Hide resolved
arvo/eyre/scry.md Outdated Show resolved Hide resolved
arvo/eyre/tasks.md Outdated Show resolved Hide resolved
arvo/eyre/tasks.md Outdated Show resolved Hide resolved
arvo/eyre/tasks.md Show resolved Hide resolved
arvo/eyre/external-api-ref.md Show resolved Hide resolved
tinnus-napbus and others added 16 commits July 21, 2021 14:43
Co-authored-by: poprox <51337059+drbeefsupreme@users.noreply.github.com>
Co-authored-by: poprox <51337059+drbeefsupreme@users.noreply.github.com>
Co-authored-by: poprox <51337059+drbeefsupreme@users.noreply.github.com>
Co-authored-by: poprox <51337059+drbeefsupreme@users.noreply.github.com>
Co-authored-by: poprox <51337059+drbeefsupreme@users.noreply.github.com>
Co-authored-by: poprox <51337059+drbeefsupreme@users.noreply.github.com>
Co-authored-by: poprox <51337059+drbeefsupreme@users.noreply.github.com>
Co-authored-by: poprox <51337059+drbeefsupreme@users.noreply.github.com>
Co-authored-by: poprox <51337059+drbeefsupreme@users.noreply.github.com>
Co-authored-by: poprox <51337059+drbeefsupreme@users.noreply.github.com>
Co-authored-by: poprox <51337059+drbeefsupreme@users.noreply.github.com>
Co-authored-by: poprox <51337059+drbeefsupreme@users.noreply.github.com>
Co-authored-by: poprox <51337059+drbeefsupreme@users.noreply.github.com>
Co-authored-by: poprox <51337059+drbeefsupreme@users.noreply.github.com>
Co-authored-by: poprox <51337059+drbeefsupreme@users.noreply.github.com>
@tinnus-napbus
Copy link
Contributor Author

all the issues raised by @drbeefsupreme have been fixed

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants