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 opt-in props for pingback #330

Closed
wants to merge 6 commits into from
Closed

Conversation

LBBO
Copy link

@LBBO LBBO commented Nov 29, 2022

This PR adds a optInToTelemetry prop to all components that can cause any pingback events. By default, it is set to false, creating an opt-in behavior. Pingback events are only sent when users explicitly opt-in.

I added e2e tests for this behavior for the react-components. Since the normal components didn't seem to have a pre-existing test setup, I didn't add any here. Of course, I also added documentation for the new property.

Also, in an effort to follow the boyscout rule, I added npm test scripts to packages that didn't have it yet so npm test can now be executed globally. Let me know if you would prefer me to extract those changes into a separate PR.

Fixes #291

@LBBO LBBO mentioned this pull request Nov 29, 2022
15 tasks
@LBBO
Copy link
Author

LBBO commented Nov 29, 2022

If I understand the error message correctly, an API key for Percy is missing. Is there anything I can do to fix that issue?

@LBBO
Copy link
Author

LBBO commented Nov 29, 2022

image

This is a desperate and experimental attempt to fix the pipeline,
since it works for that branch
@giannif
Copy link
Collaborator

giannif commented Nov 29, 2022

@LBBO thank you for trying to do this but it's not something we want to add. Disabling pingbacks by default at a component level could lead to unpredictable reporting. For example, we'd have to go through every project GIPHY has released an add this property to all of our components. This is hundreds of components. We will add a way to disable pingbacks but it must be done by the GIPHY team and be well thought-out and planned. Thank you for the PR though

@LBBO
Copy link
Author

LBBO commented Nov 29, 2022

Would it be an option to convert the opt-in to an opt-out? That way, no existing behavior would change and the privacy feature would still be added. If not, can you provide a timeline on when the feature will be addressed? I would love to have this for the Signal PR referenced above.

@giannif
Copy link
Collaborator

giannif commented Nov 29, 2022

@LBBO I think it'd be a global setting and it'd be on be default. We have a way to configure the pingback url, it'd be something along those lines and it'd be checked only once in the send-pingback function

@LBBO
Copy link
Author

LBBO commented Nov 29, 2022

And that would also be managed in the PingbackContext, right? I considered doing that in the first place but decided against it since I didn't want to force people to add elements to their shadow Dom.

But yeah, if having it be an opt-out property in the context is what you would like to see, I can start working on that tomorrow. What do you think about the current naming? Would disableTelemetry be okay or would you prefer something else?

@LBBO
Copy link
Author

LBBO commented Nov 30, 2022

Alright, I just had a look at the code and I see that you mean an actual global property stored in window or global. Why did you choose to implement it that way? Personally, I never like mutating the global scope as it can lead to weird side-effects, it enables fingerprinting and doesn't allow fine-grained control (such as using different configurations for different components within the same site).

Also, the way that the configurable pingback URL is currently implemented, the global scope must be modified before send-pingback.ts is imported as the final URL is determined immediately after import and cannot be changed from the outside. If the opt-out was implemented the same way, this would significantly restrict where the code to opt-out could be placed, which I would like to avoid.

If it's okay with you, I'd like to add a disableTelemetry property to the PingbackAttributes. Callers can then decide if they want to set it or not (default will be true, as you requested) and they can either provide it directly or via a PingbackContext. And if you'd like, I could add a check for a global property as well before defaulting to true, so it could be something like const disableTelemetry = attributes.disableTelemetry ?? gl.GIPHY_DISABLE_TELEMETRY ?? true. But I would prefer checking gl on every call, not just at import, if that's okay with you.

@giannif
Copy link
Collaborator

giannif commented Nov 30, 2022

@LBBO thanks for the feedback. Definitely aware of the issues with using a global variable, and aware that it's hard to configure. I struggled with using a env variable to configure font loading in @giphy/js-brand. However, this is a per-site/app configuration and we don't need the granularity of a context wrapper. I'd like to stick with the global variable approach and if needed do something more complicated as a follow up. Also, I still have to get the okay that this is a feature we can add as the pingback stuff is not the domain of the SDK team

@giannif giannif closed this Nov 30, 2022
@LBBO
Copy link
Author

LBBO commented Nov 30, 2022

I'm sad to hear that. Let me know when there's an update :-)

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.

Consider making pingback and remote fonts configurable
2 participants