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

SceneQueryRunner: make some fields and methods protected #745

Closed
wants to merge 1 commit into from

Conversation

sd2k
Copy link
Contributor

@sd2k sd2k commented May 24, 2024

PR #587 is quite a large change and introduces more concepts to scenes, so isn't easy to justify merging right now. As a workaround, I'd like to be able to subclass and override some methods of the SceneQueryRunner to implement some similar behaviour in a different library, but I need various fields and methods to be accessible to the subclass, which this PR does by making them protected rather than private.

Note: I can almost just completely copy/paste the class into my own code rather than extending it, but the getQueriesForVariables function looks for all instances of SceneQueryRunner, which my copy/pasted class would not match (unlike a subclass, which should), so I think some ad-hoc variable support would break :(

I'm hoping this is a less controversial change than #587 which will unblock me - let me know what you think!

PR #587 is quite a large change and introduces more concepts to scenes, so
isn't easy to justify merging right now. As a workaround, I'd like to be
able to subclass and override some methods of the SceneQueryRunner to
implement some similar behaviour in a different library, but I need
various fields and methods to be accessible to the subclass, which this
PR does by making them protected rather than private.

Note: I can _almost_ just completely copy/paste the class into my own
code rather than extending it, but the [`getQueriesForVariables`] function
looks for all _instances_ of SceneQueryRunner, which my copy/pasted class
would not match (unlike a subclass, which should), so I think some
ad-hoc variable support would break :(

`getQueriesForVariables`: https://github.com/grafana/scenes/blob/07c5dc66746d36208cd121b938883db8ef2243f7/packages/scenes/src/variables/utils.ts#L88-L94
Copy link
Member

@dprokop dprokop left a comment

Choose a reason for hiding this comment

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

@sd2k - please have a look at my review of #587. I would prefer we push the other PR forward rather than introducing this change

@sd2k
Copy link
Contributor Author

sd2k commented Jun 5, 2024

Closing, not needed now that #587 has been merged 🎉

@sd2k sd2k closed this Jun 5, 2024
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