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

Unify GRPC client docs #168

Merged
merged 32 commits into from Dec 30, 2020
Merged

Unify GRPC client docs #168

merged 32 commits into from Dec 30, 2020

Conversation

oskardudycz
Copy link
Contributor

@oskardudycz oskardudycz commented Dec 7, 2020

  • Copied docs from the .NET Client
  • Added missing NodeJS Samples
  • Fixed current NodeJS to work, configured ES6 modules.
  • Added missing .NET GRPC client samples

@oskardudycz oskardudycz changed the title WIP: Unify GRPC client docs Unify GRPC client docs Dec 21, 2020
@oskardudycz oskardudycz marked this pull request as ready for review December 23, 2020 09:26
@mat-mcloughlin
Copy link
Contributor

  • We need to sort the menu and remove the old .net docs before we publish this PR
  • The getting started menu feels off. I don't think the first thing a user should be presented with is the connection string. I know its the client section but something information about getting the server running would be good
  • I'm not sure about the jsonEvent. If we keep it we need a way to document this inconsistencies
  • No concept of "StreamState" in the JS client?
  • Image on the writing events doesn't resolve
  • Also the menu item should probably be renamed to "Appending Events"
  • Inconsistencies between "data" and "payload", "type" and "eventType"
  • Now that we are publishing we should avoid leaving todo's and broken links (options and user credentials)
  • Should we be referring to it as a database or a store?
  • Inconsistencies between "resolveLinkTos" and "resolveLinks"
  • Is there no better way to check for a stream being present than throwing an exception in JS?
  • Noticed when subscribing to a stream in JS that you only provide a revision not a commit/prepare position

@George-Payne
Copy link
Member

https://deploy-preview-168--developers-eventstore.netlify.app/clients/grpc/appending-events/#eventid

  • of of
    image

  • For most scenarios you can just provide Uuid.NewUuid() ...

    This wording is quite c# specific, the node client will just create the id for you, so you can omit it "for most scenarios"

  • ANY is the default, and isn't especially relevent here, so might be worth changing the code sample to just

    await client.appendToStream("same-event-stream", event);
    
    // attempt to append the same event again
    await client.appendToStream("same-event-stream", event);

    for clarity

  • Broken image (might just be the preview):
    image

  • Do we need to update the naming in the node client for these properties (eventType -> type, payload -> data )?

  • What is "A byte encoded representation of your event." in js land?
    image

https://deploy-preview-168--developers-eventstore.netlify.app/clients/grpc/reading-events/reading-from-a-stream.html#reading-from-a-stream

  • No such thing as an ulong in js.

https://deploy-preview-168--developers-eventstore.netlify.app/clients/grpc/subscribing-to-streams/filtering.html

  • The node client has some helper functions:
const prefixes = ["customer-"];
- const filter = { filterOn: EVENT_TYPE, prefixes };
+ const filter = eventTypeFilter({ prefixes }); 

Fixed typo and missing image in appending-events/README.md
@oskardudycz
Copy link
Contributor Author

oskardudycz commented Dec 28, 2020

@George-Payne

* of of

Fixed in 075e013#diff-1e19e6c67c68a571656c7425340cce9e7981c98cc1e80c3f12969f46745575faR34

* > For most scenarios you can just provide Uuid.NewUuid() ...
  
  
  This wording is quite c# specific, the node client will just create the id for you, so you can omit it "for most scenarios"

I removed the whole sentence, as it didn't bring much value (even for C# dev) 70da36c.

* `ANY` is the default, and isn't especially relevent here, so might be worth changing the code sample to just

Changed in 8f969b7#diff-2e65e44b56dcbda89b84f68489f70b28e7dddc3e75a78d513239e2bdc93b7183L38.

* Broken image (might just be the preview):

Fixed in 075e013#diff-1e19e6c67c68a571656c7425340cce9e7981c98cc1e80c3f12969f46745575faR51.

* Do we need to update the naming in the node client for these properties (`eventType` -> `type`, `payload` -> `data` )?

Yes, we do. I added PR for that EventStore/EventStore-Client-NodeJS#102.

* What is "A byte encoded representation of your event." in js land?

Rephrased in 8f969b7. In general, it means that even if you store the data as JSON, then eventually they'll be stored as encoded bytes.

* No such thing as an `ulong` in js.

Changed this phrase to "big int (unsigned 64 bit integer)" in 5f93962.

* The node client has some helper functions:

Thanks, I wasn't aware of them. Used them in 1408dcf.

@oskardudycz
Copy link
Contributor Author

@mat-mcloughlin

* We need to sort the menu and remove the old .net docs before we publish this PR

* The getting started menu feels off. I don't think the first thing a user should be presented with is the connection string. I know its the client section but something information about getting the server running would be good

* I'm not sure about the jsonEvent. If we keep it we need a way to document this inconsistencies

I created a separate issue for that #182

* No concept of "StreamState" in the JS client?

Currently, it's a set of variables. I think that we could group them together StreamState. That could make it more readable, but as for now it's defined as such: https://github.com/EventStore/EventStore-Client-NodeJS/blob/master/src/constants.ts#L2. @George-Payne thoughts?

* Image on the writing events doesn't resolve

Updated in 075e013#diff-1e19e6c67c68a571656c7425340cce9e7981c98cc1e80c3f12969f46745575faR51.

* Also the menu item should probably be renamed to "Appending Events"

I created a separate ticket for that, as this is global docs issue (server docs also use write instead of append. #180

* Inconsistencies between "data" and "payload", "type" and "eventType"

Addressed that in the PR: EventStore/EventStore-Client-NodeJS#102

* Now that we are publishing we should avoid leaving todo's and broken links (options and user credentials)

Added missing user credentials sample during append in c58bf5c.

* Should we be referring to it as a database or a store?

Good question - I think that database. Should I create an issue for that?

* Inconsistencies between "resolveLinkTos" and "resolveLinks"

Should I update JS client to use resolveLinkTos instead of resolveLinks? Similar question for updating FORWARD to FORWARDS. Personally, I prefer more JS naming, but changing the .NET Client will be breaking (we could provide also rolling update with marking obsolete etc.).

* Is there no better way to check for a stream being present than throwing an exception in JS?

I'm not aware of such. @George-Payne could you confirm?

* Noticed when subscribing to a stream in JS that you only provide a revision not a commit/prepare position

Yup, it's not - should I create a ticket in the NodeJS client?

@mat-mcloughlin
Copy link
Contributor

Should I update JS client to use resolveLinkTos instead of resolveLinks? Similar question for updating FORWARD to FORWARDS. Personally, I prefer more JS naming, but changing the .NET Client will be breaking (we could provide also rolling update with marking obsolete etc.).

resolveLinkTos is well defined within event store now. Although I agree with you that resolveLinks is better I think we best keep it the same for now

  • Noticed when subscribing to a stream in JS that you only provide a revision not a commit/prepare position

Yep we need an issue raising for this

  • Should we be referring to it as a database or a store?

Agree on database. Raised an issue for defining what terms we should use going forwards

The only thing that needs fixing before merging this is fixing the menu so that people don't get confused when we push the change

@George-Payne
Copy link
Member

George-Payne commented Dec 29, 2020

Noticed when subscribing to a stream in JS that you only provide a revision not a commit/prepare position

Yep we need an issue raising for this

If it is incorrect, it needs to be raised upstream, as this is how it is defined in the proto:

image

@mat-mcloughlin
Copy link
Contributor

mat-mcloughlin commented Dec 29, 2020

It's a single uint64 for subscribing to a stream

It's a position made up of a commit and prepare position when subscribing to $all

@George-Payne
Copy link
Member

George-Payne commented Dec 29, 2020

It's a single uint64 for subscribing to a stream
It's a position made up of a commit and prepare position when subscribing to $all

Seems correct:

image
image
image

- renamed payload to data, eventType to type to align with the .NET Client naming
- renamed Directions from FORWARD to FORWARDS and BACKWARD to BACKWARDS
- renamed resolveLinks into resolveLinkTos
@George-Payne
Copy link
Member

  • No concept of "StreamState" in the JS client?

Currently, it's a set of variables. I think that we could group them together StreamState. That could make it more readable, but as for now it's defined as such: https://github.com/EventStore/EventStore-Client-NodeJS/blob/master/src/constants.ts#L2. @George-Payne thoughts?

If I'm understanding the question correctly:

Constants and types tend to be the preferred way to go in ts / js libs as enums don't really exist in js, and typescript enums are a bit odd once converted to js. (This is one of the few places typescript diverges from ECMA.

They are grouped by the type itself:
image

@George-Payne
Copy link
Member

George-Payne commented Dec 30, 2020

  • Is there no better way to check for a stream being present than throwing an exception in JS?

I'm not aware of such. @George-Payne could you confirm?

It currently throws an error because thats what GRPC does (the error gets wrapped).

We could follow the way the .net one works. Doesnt seem that strange to me to throw an error, but if trying to read streams that don't exist is an expected non-error code path, then we can just catch the error behind the scenes and return a "failed" result.

@oskardudycz
Copy link
Contributor Author

@mat-mcloughlin @George-Payne I fixed the subscribeToAll samples by using proper offset with fromPosition instead of the single number in fromRevision 2396070.

@alexeyzimarev @mat-mcloughlin I commented out TODOs in 158afa7.

@oskardudycz
Copy link
Contributor Author

@George-Payne @mat-mcloughlin

We could follow the way the .net one works. Doesnt seem that strange to me to throw an error, but if trying to read streams that don't exist is an expected non-error code path, then we can just catch the error behind the scenes and return a "failed" result.

I personally prefer to return the result instead of throwing an exception. It gives a smoother experience when you want to conditionally handle that.

@oskardudycz oskardudycz merged commit 6f88f7d into master Dec 30, 2020
@oskardudycz oskardudycz deleted the feature/unify-grpc-docs branch December 30, 2020 10:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants