growthbook / devtools

GrowthBook DevTools extension for Chrome
MIT License
5 stars 5 forks source link

Add Storybook and plop generators #13

Closed tinahollygb closed 1 year ago

tinahollygb commented 1 year ago

The current developer workflow is less than ideal, requiring developers to reload the extension in the extension configuration section.

These changes add Storybook so we can use live reloading. Provided our components don't have any requirements on any Chrome extension API's, this will work. This can be achieved by using presentation-container component architecture and only mounting presentational components in Storybook.

I've added the ChakraProvider so Chakra styles can apply to our own components. Also loaded the global.css stylesheet which appears to be working. I haven't tested the Tailwind support but it seems to build.

These changes also fix a bug that causes an error to log during development only (for not having a key on mapped components).

Closes https://github.com/growthbook/devtools/issues/12

image

It also seems to load up all the Chakra UI components into Storybook by default, which is nice, for reference. This doesn't add any new code to our project, it must be scanning the node_modules for the default Storybook configuration.

image
tinahollygb commented 1 year ago

Thanks for the feedback @bttf. Overall I agree that we do need to be able to host the dev tools panel, the popup, and options pages, but I disagree with blocking this merge, as I think Storybook is a tool that can help us get to those goals faster, as well as get the feature work done we need to get done.

The main goal is to improve DX of the app in general (re-instate hot-reloading, which we lost when we switched to a webpack-based build)

That's a good goal to have, but that is not the goal of this PR. The goal of this PR is to allow me to build out the new functionality I need to build out, and setting up Storybook is the quickest way to do that.

I think your concern is valid and we should add that to the issues. It looks like there's an existing issue open for new architecture, perhaps you can handle that in that ticket since you were working on that. The responsibility of that ticket is changing the build and the architecture, which seems like a great place to handle that.

My goal with this set of changes is so that I can build faster and build the new set of functionality in isolation that I need to build. To quote the Storybook site:

Build UI components and pages in isolation Implement components and pages without needing to fuss with data, APIs, or business logic.

You can read more about all the problems Storybook solves: https://storybook.js.org/docs/react/get-started/why-storybook

Allowing us to serve the various parts of the app is a good idea that I 100% agree with, but would require a substantial refactor. Doing so would require us to refactor the app to follow presentational-container component architecture, and Storybook is a tool to help with that.

Adding Storybook does not add to the bundle and it does not get in the way of us being able to eventually be able to serve the dev tools panel, it's an optional server developers can run alongside the real one (which is what I've been doing). It actually helps us in that respect since it gives us a place to build out and test the presentational aspects that don't rely on chrome.

I would argue this is a good set of tools that should be merged. Do you feel Storybook can detract from, or take away from or goals, or negatively impact the tooling? If not, I'd recommend merging it and tackling the larger architectural concerns in the future.

jdorn commented 1 year ago

I think both Storybook and Webpack dev server are valuable for different things. Storybook for isolated component UX, Dev server for bigger changes. So I think we should do both in the long run.

Given the strict deadline for version 2.0 and the visual editor PR, my preference would be to wait to merge this until after the release.

bttf commented 1 year ago

This branch has gone stale. Deleting