api-platform / admin

A beautiful and fully-featured administration interface builder for hypermedia APIs
https://api-platform.com/docs/admin/
MIT License
480 stars 130 forks source link

Make the development process easier with Storybook #518

Closed adguernier closed 5 months ago

adguernier commented 9 months ago
Q A
Branch? main for features
Tickets null
License MIT
Doc PR ./CONTRIBUTING.md

This PR attempts to help newcomers to develop easily the Admin client, without the need to link source and a dedicated project with yarn link. Instead, newcomers could develop directly by cloning this repository and by running a make command.

The idea:

In this logic, we could add as many backends as scenarios. For instance, we could add a story and a protected backend to test that React-admin's authProvider workflow works as expected.

Branch:

TODO:

alanpoulain commented 9 months ago

Hello Adrien, Thanks for your PR. It's a good idea, but in my opinion it would increase the maintenance burden. I would prefer if we could find a workflow by using the demo instead. Or a simpler backend. Also why using Storybook? We don't have a lot of components like React-Admin, I think using Storybook does not add a lot of value.

fzaninotto commented 9 months ago

Hi Alan,

Happy return ;)

Adrien works with the core team at Marmelab. We've discussed how to improve the API Platform admin developer experience, because we need a way to test various features in isolation.

The current setup doesn't allow this. Using the demo (if you think about the e-commerce demo) won't do it either : the demo doesn't use guessers. And if you think about the current demo, it doesn't allow to test the connected setup.

Storybook will let us test each guesser independently, with all their variants. It's the solution we use on react-admin. It's not heavy, it's super reactive, and it leads to a better design of each component as we have to think about their individual API.

I really recommend going forward with this PR.

adguernier commented 9 months ago

Hello Alan,

Thanks for your reply :slightly_smiling_face:

As François said, I'm working with the React-Admin core team and while I was testing Api-Platform and the Api-Platform-Admin client, I was facing some issues. As the Api-Platform-Admin client uses React-Admin, we would like to open PRs to fix them. But first, we thought about making a plug-n-play environment and hence facilitating the development process.

So why using Storybook?

dunglas commented 9 months ago

I'm in favor of doing something like that instead of relying on the demo (which is a moving target, and sometimes not very stable).

I just wonder if we couldn't find a way to not "pollute" this Git repository by scripting the installation of the distribution instead of bundling it. It will be hard to maintain otherwise.

One last thing, I'm not a fan of Makefiles (except - maybe - in C/C++ projects, but even in these ecosystems we have better alternatives now, such as Bazel, or Makefiles generators such as CMake). Couldn't we just add some npm run commands instead?

fzaninotto commented 9 months ago

I just wonder if we couldn't find a way to not "pollute" this Git repository by scripting the installation of the distribution instead of bundling it. It will be hard to maintain otherwise.

I don't see this as a repo "pollution". It's required for a faster developer experience. It doesn't harm the users of the npm package, as it doesn't contain the examples.

You'll have to help us find such a way, because I don't see how we can do it. The code in this repo must at least contain the schema of the entities. If you had a CLI tool that allows to generate an API platform app based on a schema, that would allow us to remove some of that code. That being said, we'll also have to include an example with authentication, and this one requires custom code, so I don't see how it can be generated.

One last thing, I'm not a fan of Makefiles (except - maybe - in C/C++ projects, but even in these ecosystems we have better alternatives now, such as Bazel, or Makefiles generators such as CMake). Couldn't we just add some npm run commands instead?

Npm alone won't suffice: we must start both the frontend (a Vite app that can be launched with npm) and the backend (a docker compose with the API platform server and a Postgres BDD). We could script that with shell, but that'd be reinventing make IMHO.

alanpoulain commented 9 months ago

What I meant by using the demo, is cloning or downloading the API Platform Demo, not using it directly.

Alright for using Storybook, to develop or test each guesser independently.

If you had a CLI tool that allows to generate an API platform app based on a schema, that would allow us to remove some of that code.

You could try to use https://github.com/api-platform/schema-generator.

It would be great if we can avoid having the code of an API Platform application directly in this repo, we know from experience that having to maintain an app like this can be tiresome because we already do it for the distribution and the demo.

fzaninotto commented 9 months ago

You could try to use https://github.com/api-platform/schema-generator.

We've tried that and it's not enough.

We need to test how the admin app can adapt to non-basic APIs (with authentication, embedded relationships, custom getters, etc), and the generator can't allow for that.

It would be great if we can avoid having the code of an API Platform application directly in this repo

We do keep the code of example applications in the react-admin repo and it's a great developer experience: it helps us detect regressions, write upgrade guides, simulate integrations with third-parties, and more generally keep everything in one place.

Yes it can be tiresome to maintain, but not doing it means leaving undiscovered bugs, publishing breaking changes without noticing it, or publishing code samples in the doc that don't actually work with the last version of the library. Three problems that API Platform admin actually suffers from. It's because we want to fix these problems that we suggest a more secure approach ;).

alanpoulain commented 9 months ago

We need to test how the admin app can adapt to non-basic APIs (with authentication, embedded relationships, custom getters, etc), and the generator can't allow for that.

Sure I understand, I think custom code is needed too but only the necessary one.

We do keep the code of example applications in the react-admin repo and it's a great developer experience: it helps us detect regressions, write upgrade guides, simulate integrations with third-parties, and more generally keep everything in one place.

You're completely right, the only difference is that API Platform Admin is not our main project, that's why we try to reduce the maintenance burden for it. We should strive to have the best equilibrium here.

Obviously, I'm saying this only if another approach could work, maybe it's not the case and having a Symfony app is the only way.

adguernier commented 9 months ago

Our first attempt was to script the installation of API Platform via a shell file, using Symfony Docker. Something like:

# setup-api-platform.sh
# make commands come from the "Using a Makefile" section of Symfony Docker repository
git clone https://github.com/dunglas/symfony-docker.git api-platform
docker compose up -d
make composer c="req symfony/orm-pack"
docker compose up database -d
make composer c="req symfony/maker-bundle --dev"
make sf c="make:user --is-entity --identity-property-name=email --with-password -n -- User"
make sf c="make:migration -n"
make sf c="doctrine:migrations:migrate -n"
#...

This works for a simple backend, but what about more complicated setups. For instance, to configure an API protected by JWT, we need to modify several files (security.yaml, routes.yaml, api_platform.yam, etc.), and we have to perform other actions like generating public and private keys and so on. We could use patch perhaps (I tried), but the script will also become more complex. Ditto for the maintenance charge IMHO.

We therefore concluded that the best solution was to provide already configured backend according to the needs of the scenarios (simple, JWT protected, etc). This way we can keep the plug-n-play approach.

dunglas commented 9 months ago

The demo has all of this, and it is an open source project so adding new features to it is possible and encouraged.

Cloning it or requiring it as a submodule is likely the best option.

adguernier commented 9 months ago

Hello @dunglas and @alanpoulain,

The demo comes with many features and it seems very complete and useful. I tried it a month ago and I was facing issue with Keycloak. I spent a while trying to fix this with no luck. So I tried again, just now; and I'm still literally stuck in the Keyclock error page. Perhaps the issue is on my side, a wrong setup or something. Please look at the screenshot: image

So I have some interrogations in my mind:

I would be really nice if we find a way to include Storybook :slightly_smiling_face:

Thanks for reading!

alanpoulain commented 9 months ago

For Keycloak, maybe @vincentchalamon has some input on it?

You're right, you will have less freedom to add different scenario if you use the demo. But it's not impossible to add them if they have value, and it will be beneficial in the long term. For the particular use case of adding different authentication content, I'm not sure, maybe it will be nice to have both authenticated and not authenticated endpoints in the demo, WDYT Vincent?

The demo should not be broken, if the tests break things in the demo, we will fix it :slightly_smiling_face:

adguernier commented 5 months ago

close as completed by #535