admin icon indicating copy to clipboard operation
admin copied to clipboard

Make the development process easier with Storybook

Open adguernier opened this issue 1 year ago • 12 comments

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:

  • work with Storybook to get a quick result of the change we made (as React-admin does).
  • for this to work, stories rely on Api-Platform backends. These backends run throught Symfony Docker.

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:

  • main for new features

TODO:

adguernier avatar Nov 27 '23 11:11 adguernier

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.

alanpoulain avatar Nov 27 '23 11:11 alanpoulain

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.

fzaninotto avatar Nov 27 '23 12:11 fzaninotto

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?

  • to test scenarios and component such as recordRepresentation, React-Admin login page, custom authProvider, etc;
  • also, it allows adding e2e tests (with Playright for instance);
  • plus, Storybook can provide code examples to enhance the Admin documentation.

adguernier avatar Nov 27 '23 14:11 adguernier

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?

dunglas avatar Nov 27 '23 23:11 dunglas

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.

fzaninotto avatar Nov 28 '23 08:11 fzaninotto

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.

alanpoulain avatar Nov 28 '23 09:11 alanpoulain

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 ;).

fzaninotto avatar Nov 28 '23 10:11 fzaninotto

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.

alanpoulain avatar Nov 28 '23 13:11 alanpoulain

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.

adguernier avatar Nov 28 '23 13:11 adguernier

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.

dunglas avatar Nov 28 '23 13:11 dunglas

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:

  • where Symfony Docker is super simple and configurable according to our needs, I wonder how to deal with several different scenarios with the demo: some stories won't need authentication, or some of them will need another type of authentication for instance. Should we add firewalls in the demo? Some scenarios will need new entities and relationships perhaps. I'm a little worried that the demo will become huge and complicated.
  • the problem I mentioned certainly only happens to me, but this makes me wonder: if part of the demo is broken, so the related stories will be too?
  • what I find great with using Symfony Docker is that you can easily create, adapt and evolve the configurations according to the scenarios. In addition, the containers only include what is necessary (no PWA for instance)

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

Thanks for reading!

adguernier avatar Nov 29 '23 15:11 adguernier

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:

alanpoulain avatar Nov 30 '23 13:11 alanpoulain

close as completed by #535

adguernier avatar Mar 19 '24 13:03 adguernier