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

feat(plugin-tech-radar): export stateless ui component #2156

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

harley-davidson
Copy link

Allow upstream consumers to manage state independent of Backstage by exporting a stateless ui component of the tech radar. This PR includes new exports and documentation to support this functionality.

@harley-davidson harley-davidson requested a review from a team as a code owner December 8, 2024 13:20
@backstage-goalie
Copy link
Contributor

backstage-goalie bot commented Dec 8, 2024

Missing Changesets

The following package(s) are changed by this PR but do not have a changeset:

  • @backstage-community/plugin-tech-radar

See CONTRIBUTING.md for more information about how to add changesets.

Unexpected Changesets

The following changeset(s) reference packages that have not been changed in this PR:

  • /home/runner/work/community-plugins/community-plugins/.changeset/big-plants-study.md: @backstage-community/plugins

Note that only changes that affect the published package require changesets, for example changes to tests and storybook stories do not require changesets.

Changed Packages

Package Name Package Path Changeset Bump Current Version
@backstage-community/plugin-tech-radar workspaces/tech-radar/plugins/tech-radar none v1.0.0

Allow upstream consumers to manage state independent of Backstage by exporting a stateless ui component of the tech radar.

Co-authored-by: Harley Lang <[email protected]>
Signed-off-by: Harley Lang <[email protected]>
@@ -283,6 +283,19 @@ export const app = createApp({
});
```

#### Option C: Opt-out of statefullness and manage state independently

In situations where you need to integrate a radar and manage state independently of Backstage, use the stateless UI component:
Copy link
Member

Choose a reason for hiding this comment

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

it's already possible to do that. Have you tried providing your own implementation of techRadarApiRef by adding the following to apis.ts?

   createApiFactory({
      api: techRadarApiRef,
      deps: {},
      factory: () =>
        new (class MyApi implements TechRadarApi {
          async load(): Promise<TechRadarLoaderResponse> {
            return { entries: [], quadrants: [], rings: [] };
          }
        })(),
    }),

Copy link
Author

Choose a reason for hiding this comment

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

My use case does not use Backstage though I would like to take advantage of the amazing component that is written in this plugin. In such cases unless I am misunderstanding I don't believe createApiFactory will be available, and it will be up to the upstream consumer to manage state for the radar chart.

Copy link
Member

Choose a reason for hiding this comment

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

in your Backstage app you should have a packages/app/src/apis.ts file where you can add the code above that loads the data from whatever data source you want. The data will be injected into tech-radar plugin once loaded.

Copy link
Author

Choose a reason for hiding this comment

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

Hi @vinzscam, my use case is not with a Backstage app. I would like to use this component with an independent application with it's own state management and api logic. This is why this PR exports a stateless radar component.

Copy link
Member

Choose a reason for hiding this comment

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

I see! Thx for clarifying @harley-davidson, it makes sense 👍

Copy link
Member

Choose a reason for hiding this comment

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

in that case I'm not sure we need to explain this in the README, but calling @awanlin as he can have a better opinion on this

Copy link
Author

Choose a reason for hiding this comment

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

Thanks @vinzscam ! @awanlin , thoughts on how to best get this one merged?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, not going to like my answer. I don't like the idea of this going outside of Backstage as we have no way to know how it's being used and the impact other changes might have on this down the road. I'll defer to @vinzscam for final say on this but that's my thoughts.

Copy link
Author

Choose a reason for hiding this comment

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

Appreciate that @awanlin - I imagine the integrity of the plugin is more important than the underlying UI. On our end we could look to develop this type of component independently, but to offer a counterpoint, I don't think that the impact of exporting a stateless component would be a huge risk. Even if the API changed, upstream consumers could just pin to a version of the plugin until they have capacity to upgrade. However, I do understand that you want the focus to be on the plugin and the UI's usage within Backstage itself.

Copy link
Author

Choose a reason for hiding this comment

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

What are your thoughts @vinzscam ?

harley-davidson and others added 2 commits December 9, 2024 14:23
Co-authored-by: Harley Lang <[email protected]>
Signed-off-by: Harley Lang <[email protected]>
Co-authored-by: Harley Lang <[email protected]>
Signed-off-by: Harley Lang <[email protected]>
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.

3 participants