backstage / backstage

Backstage is an open framework for building developer portals
https://backstage.io/
Apache License 2.0
26.73k stars 5.52k forks source link

feat(EntityPicker): use EntityPresentationApi to display entities in the picker #24081

Open Julien-Hery opened 1 month ago

Julien-Hery commented 1 month ago

Hey, I just made a Pull Request!

Following issue #20955, I added EntityPresentationApi to the EntityPicker to have more "friendly" display of entities in the picker and a common way to display them across backstage.

Capture d’écran 2024-04-08 aΜ€ 11 32 32

For the rendering of the picker options I used <EntityDisplayName /> component and for access "outside" React JSX I used const entityPresentationApi = useApi(entityPresentationApiRef); and entityPresentationApi.forEntity(entity, {...}).snapshot.entityRef.

I'm not quite sure to fully understand how entityPresentationApi works, especially with the .snapshot and .update$ and it feels like duplicated a bit of code with multiple usage of .forEntity. I'm open to a bit of explanation and suggestions if there is a better way to do it πŸ™‚

I also found what I think is a bug in the DefaultEntityPresentationApi. When calling this.#getEntityForInitialRender the default context is not used and can throw an error for an entityOrRef without an explicit kind/namespace. I fixed it by passing the context through the function but I'm not sure if the initial behavior was intended or not.

:heavy_check_mark: Checklist

backstage-goalie[bot] commented 1 month ago

Changed Packages

Package Name Package Path Changeset Bump Current Version
@backstage/plugin-scaffolder plugins/scaffolder minor v1.20.2-next.1
github-actions[bot] commented 1 month ago

Uffizzi Ephemeral Environment - Virtual Cluster

Your cluster pr-24081 was successfully created. Learn more about Uffizzi virtual clusters To connect to this cluster, follow these steps:

  1. Download and install the Uffizzi CLI from https://docs.uffizzi.com/install
  2. Login to Uffizzi, then select the backstage account and project:
    uffizzi login
Select an account: 
  β€£ backstage
    jdoe

Select a project or create a new project: 
  β€£ backstage-6783521
  1. Update your kubeconfig: uffizzi cluster update-kubeconfig pr-24081 --kubeconfig=[PATH_TO_KUBECONFIG] After updating your kubeconfig, you can manage your cluster with kubectl, kustomize, helm, and other tools that use kubeconfig files: kubectl get namespace --kubeconfig [PATH_TO_KUBECONFIG]

Access the backstage endpoint at https://backstage-default-pr-24081-c5614.uclusters.app.uffizzi.com

github-actions[bot] commented 1 month ago

This PR has been automatically marked as stale because it has not had recent activity from the author. It will be closed if no further activity occurs. If the PR was closed and you want it re-opened, let us know and we'll re-open the PR so that you can continue the contribution!

freben commented 1 month ago

Hi again! I'm very sorry that this has taken so long, but I've now added https://github.com/backstage/backstage/pull/24595 that provides a promise-based API for presentations. This lets you fetch all presentations immediately inside the useAsync block so that you don't have to touch the presentation api anywhere else in the code. For example, build an entity-ref-to-presentation map as part of the results of the useAsync. Then they are "synchronously" available in all code.

I started rewriting this myself on top of that PR, but it turns out that the free solo mode really makes things annoying to get right (since it sometimes ends up having a state string of the presentation value, and sometimes an entity), so ... had to put it off a while and focus on other things.

If you get the time to start your work on top of that branch (if the PR isn't merged yet when you read this), then awesome. πŸ™

benjdlambert commented 2 weeks ago

@benjidotsh your PR is pretty closely related to this, does it overlap?

benjidotsh commented 2 weeks ago

@benjdlambert I'm not too familiar with EntityPresentationApi, but we definitely touch the same code. πŸ˜…

I'll see if I can look into this a little more next week.

awanlin commented 1 week ago

Hi @Julien-Hery @freben, what's needed to move this forward? Best I can tell there are some un-resolved comments. I'm just following up as this is something that the Community really would like to see in place and there is already what looks like a duplicate PR #24852 submitted for this.

freben commented 1 week ago

@ahhhndre It's using .snapshot, which almost certainly will do the wrong thing. Code should use promises, for example like here

Julien-Hery commented 1 week ago

Hello @freben πŸ™‚

I'm also sorry to not have answered quicker, I was off for 3 weeks πŸ˜… I will take a look at what you did πŸ‘

To summarize what I did and where I was blocked : freeSolo

If you select an entity in the list and then click outside the input it will trigger a blur event and parse the value of the input (which is now primaryTitle) and result as a non valid entity (without namespace and kind) I'm not sure on how to solve this, blur event is needed for the freeSolo πŸ™

I don't have any idea on how to solve that except adding a specific behavior for it but it makes things very complex for a component that should not be

If anyone has an idea maybe I can find time to work on it πŸ™‚

benjidotsh commented 1 week ago

Hi @Julien-Hery, welcome back and sorry for stealing your issue πŸ˜„

I suggest you to take a look at my PR (#24852), which is based on the changes made by @ahhhndre and the implementation of MultiEntityPicker. I think the implementation should be similar. It also makes the freeSolo thing a non-issue since it directly uses the entity ref as its value.

Julien-Hery commented 1 week ago

@benjidotsh no problem πŸ˜„

I will take a look today to see if I can merge our work and have something good πŸ’ͺ

Julien-Hery commented 1 week ago

I transfered your work to my branch @benjidotsh πŸ™‚

As I was saying here and following what you did, using the full ref as a value is the only solution to work with freeSolo.

For the last usage of humanizeEntityRef, I updated the getLabel function. The function is only used if the value from formData was not found in the catalog and if freeSolo is enabled. So the function should never received something that needs to be used with presentation api but may receive something that needs to have default namespace and kind added. I think using parseEntityRef and then stringifyEntityRef is enough in this case.

@benjidotsh you should be the one getting credit for most of this PR πŸ˜…

@freben @ahhhndre let us know what you think of this πŸ™‚

benjidotsh commented 1 week ago

Thanks @Julien-Hery, teamwork makes the dream work πŸ˜„