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

Fix branch #16

Merged
merged 28 commits into from
Apr 25, 2024
Merged

Fix branch #16

merged 28 commits into from
Apr 25, 2024

Conversation

larsve
Copy link
Contributor

@larsve larsve commented Mar 27, 2024

Contains a few fixes for various things, and also introduce two new endpoints for auth and sign that have a slightly different API that return more information to caller.

larsve added 5 commits March 26, 2024 17:24
* Move error check, to happen before the value returned is used
* (Temporarily) comment unused struct variables (should be either removed or
  used)
internal/sse/sender.go Outdated Show resolved Hide resolved
internal/sse/sender.go Outdated Show resolved Hide resolved
internal/bankid/endpoints.go Outdated Show resolved Hide resolved
internal/httpserve/bankid.go Outdated Show resolved Hide resolved
@larsve larsve changed the title [DRAFT] Fix branch Fix branch Apr 17, 2024
internal/bankid/bankid_models.go Show resolved Hide resolved
@@ -113,7 +113,7 @@ func (a *API) Change(ctx context.Context, r *ChangeRequest) (*CollectResponse, e
}
}

func (a *API) WatchForChange(ctx context.Context, orderRef string) chan WatchResponse {
func (a *API) WatchForChange(ctx context.Context, orderRef string) <-chan WatchResponse {
Copy link
Contributor

Choose a reason for hiding this comment

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

nice!

Err error
}

func (a *API) WatchForChangeV2(ctx context.Context, orderRef string) (<-chan Change, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we perhaps name this something different to highlight their intended purposes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe? My best suggestion were WatchForChanges, but I felt that it would be too like WatchForChange without clarifying the differences between them, so I just kept the V2 suffix so that (in my mind at least) it would be more obvious that it were related to the v2 endpoints, but it still performed a similar function as WatchForChange..

return 0, nil, nil
}

// Handle messages that use 'CRLF' or 'LF' line endings.
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need to handle CRLF endings?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, if it's only used to read SSE messages from the writer in twofer, then we would not need to handle either CRLF or CR delimited messages. When I did the reader, I wanted it to follow the specification, so that it would be possible to use it to read SSE messages from other sources as well. This were when I intended to use SSE for the new v2 endpoints, before i discovered NDJSON. Since I already had everything in place for SSE, I kept it around in case it's simpler to use that in other languages, but if "caller" is using GO, it's simpler and much less code to use NDJSON, so I made NDJSON the default for the new v2 andpoints, but it's still possible to switch to SSE by setting an environment variable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Come to think about it now, maybe it would have been better to pass the intended delimiter as a parameter when creating the reader, that way the reader would not need the code that try to handle all possible line-endings.. Since when you are working with an SSE-stream, you would know what delimiter that particular stream uses as delimiter.

@larsve larsve merged commit f103f17 into master Apr 25, 2024
@larsve larsve deleted the fix_branch branch April 25, 2024 09:03
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.

2 participants