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

Add laravel echo agent headers #170

Merged
merged 4 commits into from
Nov 3, 2022
Merged

Conversation

sacOO7
Copy link
Collaborator

@sacOO7 sacOO7 commented Oct 26, 2022

  • Added agent headers for laravel-echo and laravel-broadcaster

Copy link
Member

@owenpearson owenpearson left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@QuintinWillison QuintinWillison left a comment

Choose a reason for hiding this comment

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

I'm confused as to how we're using emitters. In the schema we describe it as:

An array containing the names of Ably SDKs which directly emit this identifier. SDK names must match the repository name we use in GitHub (i.e. the component that appears after ably org in the URL).

Looking at ably/laravel-broadcaster#7, which I clearly aligns with this PR (i.e. implements these product identifier emitters), my expectation is that we should be seeing:

"emitters": ["laravel-broadcaster"]

Where "type" of "wrapper" is clearly still appropriate.

@owenpearson
Copy link
Member

@QuintinWillison My interpretation of "Ably SDKs which directly emit this identifier" from that schema is SDKs which actually implement the sending of the Ably-Agent header (or param), so if laravel-broadcaster emits the agent by using the ably-php client option then it's indirectly emitting the identifier.

I think we already interpret this inconsistently in agents.json because based on my interpretation the agent for ably-flutter would be incorrect, however based on your interpretation I think the agents for ably-ruby-rest (and maybe laravel) would be incorrect.

It would be good to know more about what this field is actually used for in the backend and then make the schema more specific, @lmars WDYT?

@SimonWoolf SimonWoolf removed their request for review November 1, 2022 11:07
@lmars
Copy link
Member

lmars commented Nov 1, 2022

It would be good to know more about what this field is actually used for in the backend and then make the schema more specific

The emitters field isn't used within the system, so if we're also unsure what it is used for outside the system, perhaps just drop it from the schema?

@sacOO7
Copy link
Collaborator Author

sacOO7 commented Nov 1, 2022

Maybe rename it to root / root-sdk / base / base-sdk / parent / parent-sdk . Having ably-js or ably-php value helps to understand where the agent is exactly coming from. So, by looking at agents.json, we can identify each SDK emitting specific identifiers.

@QuintinWillison
Copy link
Contributor

In 5f47be5 I've updated this pull request to conform with the change made in #172 (removed the emitters field from JSON, moving it to the commentary in the markdown readme, keeping it in its current form in order not to change the nature of @owenpearson's approval).

I remain of the opinion that I expressed previously, so will not be approving this pull request in current state. However, it can freely merge to main (default) branch without my approval, as I now remove my objection because the values I was complaining at have moved from 'data' to 'docs' now (hence easier to correct in future and much less likely to relied upon by downstream systems).

@QuintinWillison QuintinWillison dismissed their stale review November 2, 2022 09:36

We've lessened the impact of the change I was objecting to.

@owenpearson owenpearson merged commit 372a2ef into main Nov 3, 2022
@owenpearson owenpearson deleted the feature/laravel-echo-agent-headers branch November 3, 2022 13:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants