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

Switch to automatic dependency injection of Utility APIs #2285

Merged
merged 18 commits into from
Sep 8, 2020
Merged

Conversation

Rugvip
Copy link
Member

@Rugvip Rugvip commented Sep 4, 2020

Fixes #2182

Will add doc changes to this PR once we're happy with the changes.

Previous design was a single API registry where all APIs where wired up in apis.ts, simple but tedious and a lot of code required in the App.

This switches out to a more complex setup, but where the work of wiring up plugins in moved out from the app, making it significantly easier to add plugins to an existing app. apis.ts still exists, but it's only used for overriding the default api factories shipped in @backstage/core and each plugin.

Instead of directly instantiating all APIs and startup, we now lazy-load them as they are asked for. There's a quick sanity check at startup to make sure that there are no cycles in the dependency tree, but other than that failures will be encountered later. The instantiation is still synchronous, which is very much intentional, as I don't think we want plugins to do any kind of work at startup.

Couple of naming things to discuss:

  • I opted for referring to the ApiFactories as just api in the external API. Since there's no other way to instantiate an API than using a factory, I felt that apiFactories and defaultApiFactories etc. were a bit redundant and went with the shorter form.
  • Each ApiFactory earlier had an implements field, which I renamed to api. I think api captures the intention a bit better and is a bit more readable. I'm trying to skip having to think about "creating API factories", and make the mental model be that we're creating APIs. Another option here would be apiRef, since that's the actual value, but I feel that api is nicer, and typescript should take care of avoiding any confusion there anyway. It also aligns with useApi, where you're also actually passing in an ApiRef.
  • Related to the above, the function to create ApiFactories is called createApiFactory. Here I did feel like createApi would be a bit too misleading, and I left in the Factory word just to give a hint at what it is we're actually doing.

@Rugvip Rugvip requested review from a team as code owners September 4, 2020 16:07
createApiFactory({
api: catalogApiRef,
deps: { discoveryApi: discoveryApiRef },
factory: ({ discoveryApi }) => new CatalogClient({ discoveryApi }),
Copy link
Member Author

Choose a reason for hiding this comment

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

Here's an example of what it looks like from a plugin's point of view

@Rugvip
Copy link
Member Author

Rugvip commented Sep 4, 2020

Not intending for this to build yet, will update docs and create-app etc once we've settled any discussions 😁

Please do review still :>

@Rugvip Rugvip changed the title Switch to automatic dependency injection for plugin-provided Utility APIs Switch to automatic dependency injection of Utility APIs Sep 4, 2020
@Rugvip Rugvip force-pushed the rugvip/di branch 3 times, most recently from 19e7169 to be5a9f3 Compare September 7, 2020 09:13
Copy link
Collaborator

@marcuseide marcuseide left a comment

Choose a reason for hiding this comment

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

👍

@dtuite
Copy link
Collaborator

dtuite commented Sep 8, 2020

Will this require changes in plugins?

@Rugvip
Copy link
Member Author

Rugvip commented Sep 8, 2020

@dtuite preferably, but not immediately. Plugins can still choose to not supply default factories for their APIs, but then they have to be wired up in the app like before.

e.g. https://github.com/spotify/backstage/blob/62c2971a4aa923c1a49982d5bf2cba0145c62525/packages/app/src/apis.ts#L70

For the Travis and GH PRs plugins to be updated we'll need a new release with these changes, so that they can start exposing the factories, and then remove them from the app.

@dtuite
Copy link
Collaborator

dtuite commented Sep 8, 2020

@Rugvip we will work on that once this is merged. We're also working on migrating to the new routing API and tabs on the catalog entity page.

@Rugvip
Copy link
Member Author

Rugvip commented Sep 8, 2020

Adding docs in a followup PR so we can have a more focused review

@Rugvip Rugvip merged commit ef70177 into master Sep 8, 2020
@Rugvip Rugvip deleted the rugvip/di branch September 8, 2020 16:51
@Rugvip
Copy link
Member Author

Rugvip commented Sep 8, 2020

#2334

@dtuite
Copy link
Collaborator

dtuite commented Sep 9, 2020

By the way, it would be great to get a release cut soon so people can more easily migrate plugins which are not in the monorepo over to the new APIs. 🤞

@Rugvip
Copy link
Member Author

Rugvip commented Sep 9, 2020

@dtuite Yep, just making sure #2200 is all good before we release, i.e. waiting for that to be closed.

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.

Use automatic dependency injection for plugin-provided Utility APIs
5 participants