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

Remove Plugin::activate? #1549

Closed
SimonSapin opened this issue Aug 19, 2022 · 3 comments · Fixed by #1569
Closed

Remove Plugin::activate? #1549

SimonSapin opened this issue Aug 19, 2022 · 3 comments · Fixed by #1569
Assignees

Comments

@SimonSapin
Copy link
Contributor

Since #1463 no plugin implements the activate method. Is it still useful, or can every plugin manage with doing its initialization in new? If we remove it for 1.0, we’ll still have the option of adding it back in 1.x if needed.

@BrynCooke wanted to have a look at why it was added in the first place

@BrynCooke
Copy link
Contributor

BrynCooke commented Aug 19, 2022

Its useful if a plugin has to interact with a global.

The issue is that if a plugin interacts with a global during construction then there is no way to recover if a subsequent plugin fails.

Unlike construction, activate is infallible, so interaction with globals at this point is safe.

I would not bank on hot reload for telemetry staying out forever, and I can imagine that there will be other situations where interactions with globals may be necessary.

@garypen
Copy link
Contributor

garypen commented Aug 19, 2022

Those are arguments for why we might need it in the future though and, as @SimonSapin noted, it's easy to make compatible additions. I'd favour removing it until we need it.

@BrynCooke
Copy link
Contributor

I was thinking more about users which are trying to interact with their own client libraries rather than us. I agree we don't have a usecase right now, but lifecycle of other libraries won't necessarily follow our plugins. Agree we can remove and wait for user to ask for it though.

@garypen garypen self-assigned this Aug 22, 2022
garypen added a commit that referenced this issue Aug 22, 2022
fixes: #1549

also remove plugin_muts() from supergraph_service since the activate
functionality was the only consumer of this API.
@SimonSapin SimonSapin added this to the v1.0.0-alpha.0 milestone Aug 23, 2022
garypen added a commit that referenced this issue Aug 23, 2022
* remove activate() from the plugin API

fixes: #1549

also remove plugin_muts() from supergraph_service since the activate
functionality was the only consumer of this API.
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 a pull request may close this issue.

3 participants