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

Make the development process easier with Storybook #518

Closed
wants to merge 1 commit into from

Conversation

adguernier
Copy link
Contributor

@adguernier adguernier commented Nov 27, 2023

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 adguernier changed the title Dev add storybook Make the development process easier with Storybook Nov 27, 2023
@alanpoulain
Copy link
Member

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
Copy link
Contributor

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
Copy link
Contributor Author

adguernier commented Nov 27, 2023

Hello Alan,

Thanks for your reply 🙂

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 adguernier marked this pull request as ready for review November 27, 2023 15:03
Copy link
Contributor

@slax57 slax57 left a comment

Choose a reason for hiding this comment

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

Otherwise it looks good 🙂
I still would like to test it locally though, so my review is WIP.

Comment on lines +30 to +31
- by linking `@api-platform/admin` sources to an existing project;
- by installing this project and running it through Storybook.
Copy link
Contributor

Choose a reason for hiding this comment

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

These should link to their respective doc section (to act as a kind of TOC)


Create your client that will use `api-platform-admin` (replace `<yourproject>` by your project's name):
> Prerequiste: running Api-Platform backend. See the [Getting Started guide](https://api-platform.com/docs/distribution/) to learn more.
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this sill necessary? If I'm not mistaken you embedded a way to create one directly in this repo, didn't you?


Replace `src/App.js` by this one:
Your client should already uses `@api-platform/admin` and its bootstrap file (usually: `src/App.tsx`) should at least contains:
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Your client should already uses `@api-platform/admin` and its bootstrap file (usually: `src/App.tsx`) should at least contains:
Your client should already use `@api-platform/admin` and its bootstrap file (usually: `src/App.tsx`) should at least contains:


#### Running Admin through Storybook

If you don't have an existing API Platform application, you can use one of the bundled example APIs, and visualize the admin through [Storybook](https://storybook.js.org/).
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
If you don't have an existing API Platform application, you can use one of the bundled example APIs, and visualize the admin through [Storybook](https://storybook.js.org/).
If you don't have an existing API Platform application, or don't want to use `yarn link`, you can use one of the bundled example APIs, and visualize the admin through [Storybook](https://storybook.js.org/).

Get the source of `@api-platform/admin`:

```shell
cd ..
Copy link
Contributor

Choose a reason for hiding this comment

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

cd .. doesn't seem necessary to me since we won't need to move back and forth between two projects in this case, we only work with the admin repo

Comment on lines +14 to +18
start-simple: ## Start the simple Api-Platform backend and the Storybook frontend
cd backend/simple && make build && make up HTTP_PORT=${SIMPLE_HTTP_PORT} HTTP3_PORT=${SIMPLE_HTTPS_PORT} HTTPS_PORT=${SIMPLE_HTTPS_PORT} && cd .. && yarn run storybook

stop-simple: ## Stop and prune the simple Api-Platform backend
cd backend/simple && make prune
Copy link
Contributor

Choose a reason for hiding this comment

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

You should also provide targets to

  • only start the embedded backend
  • only start the storybook, without starting a backend (in case the user are providing their own backend)

@dunglas
Copy link
Member

dunglas commented Nov 27, 2023

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?


Create your client that will use `api-platform-admin` (replace `<yourproject>` by your project's name):
> Prerequiste: running Api-Platform backend. See the [Getting Started guide](https://api-platform.com/docs/distribution/) to learn more.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
> Prerequiste: running Api-Platform backend. See the [Getting Started guide](https://api-platform.com/docs/distribution/) to learn more.
> Prerequiste: running API Platform backend. See the [Getting Started guide](https://api-platform.com/docs/distribution/) to learn more.

@fzaninotto
Copy link
Contributor

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
Copy link
Member

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
Copy link
Contributor

fzaninotto commented Nov 28, 2023

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
Copy link
Member

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
Copy link
Contributor Author

adguernier commented Nov 28, 2023

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
Copy link
Member

dunglas commented Nov 28, 2023

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
Copy link
Contributor Author

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 🙂

Thanks for reading!

@alanpoulain
Copy link
Member

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 🙂

@soyuka soyuka mentioned this pull request Mar 6, 2024
@adguernier
Copy link
Contributor Author

close as completed by #535

@adguernier adguernier closed this Mar 19, 2024
@adguernier adguernier deleted the dev-add-storybook branch March 19, 2024 13:10
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.

5 participants