Skip to content

Add a Prototype for API Documentations#1737

Merged
Rugvip merged 13 commits into
backstage:masterfrom
SDA-SE:feat/api-catalog
Aug 13, 2020
Merged

Add a Prototype for API Documentations#1737
Rugvip merged 13 commits into
backstage:masterfrom
SDA-SE:feat/api-catalog

Conversation

@dhenneke

@dhenneke dhenneke commented Jul 24, 2020

Copy link
Copy Markdown
Contributor

Here is our first implementation for the API documentation plugin aka api-docs. See here for the initial discussion by @Fox32.

For now API entities can be added to the catalog and referenced from a component. They are then displayed under the API tab in the details view. In addition, a standalone API list is added. See the screenshots for some details:

We ran into some problems that are not solved till now. Probably these are things we can't resolve right now, but should be the next steps:

  • We added two external react components to display the documentation. Styling of these components could be improved by introducing own CSS files. Especially dark mode isn't supported right now. The question is, how can we style it using the backstage theme?
  • Right now it is difficult to add new plugins to the catalog details. We added it directly, but it feels like quite a hard dependency. In addition the tab navigation isn't ready yet, but others are struggeling too.
  • Adding a API list to the plugin would be nice, but brings some problems. We would have a cycling dependency between the plugin and the catalog-plugin (the scaffolder-plugin has the same problem). Do we need a package that contains central catalog components like the catalogAPiRef?
  • We added AsyncAPI support, but have trouble running the jest tests with it. It's not clear what exactly the error during test execution means: "Jest encountered an unexpected token"
  • An ownership property for the API entity might be interesting.

We know the PR is quite big but we think that it is a good start for discussions on a working prototype. If you are interested, we can split it up a bit more along the commits and contribute the changes in individual PRs.

@dhenneke dhenneke requested a review from a team as a code owner July 24, 2020 14:52
@stefanalund

Copy link
Copy Markdown
Contributor

Rockstars. Thanks a lot! 💯🥳👏🏼

@Rugvip Rugvip left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thank you! Stellar stuff 😁 🌟

High quality all around, super happy to merge and work out any specific details later.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It's silly, but best to return a cancel function here in case we get instantly unmounted :p

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

👍

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Happy to merge but no promises on keeping this 😅

It does look to me like there's opportunity for creating a pattern for how to define relationships to other entities. Maybe even a separate top-level object and a more list-style approach with room for metadata about the relationship. Or just something more simple like this. Totally not in scope of this PR though

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes. Since ADR 005 also intends the "consumes" relation it might be good to have a more general approach in the future. As soon as some kind of versioning is introduced, this simple modeling will also not suffice. But we will see then.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

jest.mock can be placed below the imports as well, so I'm wondering why you place it here? Asking because I'm considering switching to a faster build that doesn't support that hoisting and this would have to be done everywhere 😁

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I copied it from the catalog plugin 😀. But I can change it.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Dammit, ok 😁

It's fine to leave as is

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Neat 👌

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This definitely needs a nicer API (in core) imo, but not for this PR 😅

Comment thread plugins/catalog-backend/package.json Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Tbh I'm not sure why the local isn't the only one. Let's re-evaluate after merging this :>

@mbruggmann

Copy link
Copy Markdown
Contributor

Can't really comment on the TypeScript implementation, but from a modelling/metadata perspective this looks pretty great! As @Rugvip mentioned, we might want to give the spec.implementsApis part another thought but I'd vote to start with this for now and re-evaluate later if we need to. Thanks so much for getting this going!

@dhenneke

Copy link
Copy Markdown
Contributor Author

I fixed the issue with the failing build by updating the jest config (see last commit):

- '/node_modules/(?!.*\\.(?:esm\\.js|bmp|gif|jpg|jpeg|png|frag|xml|svg)$)',
+ '/node_modules/(?!@kyma-project/asyncapi-react/)(?!.*\\.(?:esm\\.js|bmp|gif|jpg|jpeg|png|frag|xml|svg)$)',

The npm package seems to be a bit odd...

I am not sure if this is a smart solution, especially since I changed in the cli package :-/.

Comment thread packages/cli/config/jest.js Outdated

@Rugvip Rugvip Jul 28, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yeah this is idd unfortunate. The package is using esm modules but really doesn't signal that in any way as far as I can tell. I don't wanna block this PR because of the issue so fine with override for now, but we do need to figure out a long-term solution. It would be possible to add this to .jest.transformIgnorePatterns inside package.json of the api-docs package, but that would have to be kept in sync with updates to this config so it isn't great either.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@Rugvip I can also try to fix the original package so we don't get an issue here. But tbh, I am not really deep into jest and esm modules. Do you have a clue what might be wrong there? Would you say this issue is specific to the configuration in the backstage repository?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It's a somewhat common override that's needed when using jest. The package is built in a way that makes it tricky to consume, and could definitely be left as feedback to the package author.

A common way to solve the issue is to build both CommonJS and ESModule outputs, and then point to each using main and module fields in package.json. It's puts more requirements on the build tooling though. For example, we do that here: https://github.com/spotify/backstage/blob/c4b6c1397577b89f117a2f663088315dda75aaa7/packages/catalog-model/package.json#L11

@Fox32

Fox32 commented Aug 7, 2020

Copy link
Copy Markdown
Contributor

Just to let you know: we are still interested, but on vacation right now. We will continue with it next week 😊

@Rugvip

Rugvip commented Aug 7, 2020

Copy link
Copy Markdown
Member

@Fox32 thank you for the update! Many vacations on our side too 😁

@dhenneke dhenneke force-pushed the feat/api-catalog branch 3 times, most recently from b642972 to 91b9ece Compare August 12, 2020 06:36
@stefanalund

Copy link
Copy Markdown
Contributor

@dhenneke what is the status of this PR? Excited to get it merged 😃

@dhenneke

Copy link
Copy Markdown
Contributor Author

The build should work now. Strangely, the "Frontend CI" workflow has not been triggered so the tests weren't executed 😕. I wonder if it is connected to 9d1f531.

Apart from that: Are there any blockers before merging this PR? If our changes in the Entity-tab-behavior are conflicting, we will be happy to remove them in favor of the work done in #1835. We could integrate that separately after that has been merged. Just leave a short note on what is missing on the road of getting this merged.

@Rugvip

Rugvip commented Aug 12, 2020

Copy link
Copy Markdown
Member

@dhenneke Will have a proper look in a bit, but iirc we're just waiting for conflicts to be resolved and should then be good to merge 👍

Workflows are indeed not triggering correctly, fix in it's way 😅 #1918

Can't comment on the tab implementation, what do you think @hooloovooo ?

@dhenneke dhenneke force-pushed the feat/api-catalog branch 4 times, most recently from 9fe12e7 to 1438d31 Compare August 12, 2020 11:15
@dhenneke

Copy link
Copy Markdown
Contributor Author

@Rugvip did you have time to have a look?

@Rugvip Rugvip left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

:shipit: !

CI failures are fixed in latest master, so you'll need to trigger a build on top of that

@freben freben left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is really solid work. Thank you!


## Links

- (The Backstage homepage)[https://backstage.io]

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

mixed up the square brackets and parentheses

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can fix later, let's :shipit: 😁

@dhenneke

Copy link
Copy Markdown
Contributor Author

Builds are green 😲

@Rugvip Rugvip merged commit e0632bc into backstage:master Aug 13, 2020
@dhenneke dhenneke deleted the feat/api-catalog branch August 13, 2020 11:39
@Fox32

Fox32 commented Aug 13, 2020

Copy link
Copy Markdown
Contributor

🥳

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.

6 participants