Add Storybook

[thektan]

Related to this thread on slack.

Entry devDependencies is not allowed.

We've been working on a new module (not merged in master yet) and we've been using Storybook (https://storybook.js.org/) for the frontend dev environment which has been great for working/testing components in isolation.

Here's a link to the current storybook setup we have: https://github.com/thektan/liferay-portal/tree/LPS-89524_Search_Tuning_Result_Rankings/modules/apps/portal-search/portal-search-ranking-web/src/main/resources/META-INF/resources/js/stories

• It includes loading in language keys (currently only the current module's language keys are translated) https://github.com/thektan/liferay-portal/blob/LPS-89524_Search_Tuning_Result_Rankings/modules/apps/portal-search/portal-search-ranking-web/src/main/resources/META-INF/resources/js/stories/webpack.config.js#L27-L30
• It also proxies portal to retrieve resources like icons and styles.

Since devDependencies are not allowed, would Storybook be something we'd want to add? Or possibly some other alternative that doesn't require the entire portal to need a dependency that possibly only a single module will use.

[wincent]

Some "meta" stuff, not specifically about Storybook:

---

Just to reflect some of the context from that Slack thread here, seeing as not everyone may have permission to view that link, I'll paraphrase @jbalsas who was explaining the "no devDependencies" stance:

• We have a lint in place to stop new devDependencies being added because we want a centralized/standard decision making procedure for the build process and what tools get brought in. Motivation here is:
  • Performance: as devDependencies get added download/build times increase.
  • Security: more dependencies means a larger footprint to audit, and not just the first time something is added but whenever it is updated.
  • Consistency: more dependencies mean more places where platform-specific code can cause builds to start diverging and therefore be less reproducible and harder to troubleshoot (some things may run, not run, or run differently on macOS, Windows etc, or may be sensitive to the local version of NodeJS installed etc).
• @jbalsas will figure out how/where to document the rationale and process to get new dependencies in, but for now, this repo is exactly the right place to start the kind of conversation that you have here @thektan (we are consolidating all frontend build processes around the tooling in this repo).

---

With that framing in place, let the conversation proceed! 😁

[bryceosterhaus]

This brings up a pretty good point in regards to entirely vetoing devDependencies. There are almost two different types of "devDependencies", ones that are required to build(babel, webpack, typescript, etc.) and then ones used while developing(storybook, eslint, prettier, etc.). I could imagine that we might need to allow for non-build required dependencies, but not entirely sure what the best way to go about that would be because even prettier and eslint would likely be in the veto category. But there does seem like is a need for a category that would allow for packages like storybook, where it is purely for the developer experience. I'm not entirely sure it'd make sense to pull storybook into this tool, since we would then have to evaluate each add-on as well, but I do think we might need to allow for certain devDependencies.

[wincent]

I definitely wouldn't put prettier and eslint in the same category as storybook, because we care about style and formatting, so they're something that we want to apply uniformly everywhere. In that light, I'm not sure how useful the build vs non-build distinction is useful. If our reasons for wanting to review new dependencies are performance, security, and consistency, then I think we'll still want that review regardless of which categories (build, non-build, "dev-exp", "runtime", "production" or otherwise) we use to classify the candidates; and naturally, the exact weight that we place on those criteria might vary according to the circumstances.

I think we need to be clear on what "veto" here means. My impression is that it means dev dependencies cannot be added without due review; it doesn't mean that they cannot be added. As such, precisely those kind of "developer experience" ones that you mention, I'd expect to see them reviewed/evaluated and then the ones that we think are worth it — in terms of the benefit vs the costs (performance, security, consistency) — will be allowed in.

[wincent]

BTW while you're here @bryceosterhaus, given that you've been using Storybook in Clay, do you have any specific feedback or recommendations about it here?

[bryceosterhaus]

Sorry I was probably a bit too hasty in lumping in eslint and prettier to the same category as storybook and probably didn't explain myself well. What I was trying to express was that there are dependencies that do not get shipped with the app OR are used in packaging the app. My main point I was trying to express is that we might need a category to define external dependencies that are used in a developers workflow, but not actually impacting the code itself.

However, since our aim is to help unify the frontend ecosystem in portal, it probably makes sense to be the gate keepers for all packages here since we wouldn't want one project to use storybook and then another project to spin up their own home-rolled version of storybook with express.js or something.

That being said, in my experience with Storybook so far, I am a big fan. On Loop and Analytics Cloud we rolled out our own "UI Kit" for every component and it was really helpful. If we would've used something like storybook for those apps, it would've reduced the amount of overhead in creating those demos and also removed a lot of boilerplate we ended up re-creating for every component. So I would be in favor of adding storybook as a dependency and possibly a script as well liferay-npm-scripts storybook

[thektan]

If we do add storybook, +1 to having a new script liferay-npm-scripts storybook to standardize configurations since if other portlets used storybook as well, they'd most likely have similar configurations (while still allowing portlet-specific configurations).

[bryceosterhaus]

Also one additional reason I can see us allowing for storybook is that we set an example of using it with clay. When teams decide to create their own react components, they will likely look at our storybook site for examples.

[jbalsas]

Hey guys, thanks for the conversation!

I think this is indeed something worth standardizing for all frontend at least in DXP.

@thektan, @bryceosterhaus, would you mind working on a proposal for this in line of the other commons we have? Please, try to be opinionated for these things. I know our natural tendency is the opposite, but let's see what would be a minimal, satisfactory set and start from there. Then we can follow this same process to refine it (e.g. standardizing additional storybook plugins).

Once we have a proposal, we could make a call to interested parties to review and or comment before we merge in and document it.

BTW, I collected some of the reasons about devDependencies in DXP in our tentative guidelines repo and formalized this as the workflow to get new dev dependencies approved. Please, review that doc as well and feel free to open related issues for clarification or make suggestions on it.

Thanks!

[wincent]

@thektan: I turned the title prefix into an actual tag and I'll update the guidelines accordingly.

[thektan]

@bryceosterhaus @jbalsas I'll start working on a proposal for this.

[wincent]

So the initial version of this is in v4.18.0; sent PR upstream for that here:

https://github.com/brianchandotcom/liferay-portal/pull/76696

Thanks @thektan. Epic work!