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

feat: Storybook schematics #1582

Merged
merged 15 commits into from
Oct 23, 2019
Merged

Conversation

isaacplmann
Copy link
Collaborator

Please make sure you have read the submission guidelines before posting an PR

Please make sure that your commit message follows our format.

Example: fix(nx): must begin with lowercase

Current Behavior (This is the behavior we have today, before the PR is merged)

No way to add storybook to a workspace.

Expected Behavior (This is the new behavior we can expect after the PR is merged)

@nrwl/storybook is a set of schematics to programmatically scaffold storybook stories and cypress tests to a ui lib.

TODO: Add tests for all the schematics.

function addStorybookTask(projectName: string): Rule {
return <any>updateWorkspace(workspace => {
workspace.projects.get(projectName).targets.add({
name: 'storybook',
Copy link

@kbrilla kbrilla Jul 14, 2019

Choose a reason for hiding this comment

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

what happens if we want multiple projects to have storybook? Won't we have a conflict here with the name? Btw. Should we have multiple storybooks per project or maybe only one for the whole workspace?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sorry, I should have included more information on the goal of this PR. This schematic is set up to create an isolated storybook instance for a specific ui-lib. For large monorepos, having a single storybook instance would be un-manageable.

This line doesn't cause a conflict - it creates a storybook target for the individual lib (just like each lib has its own lint target). The run-command is set to point at the specific .storybook folder inside the lib (which only searches that lib for *.stories.ts files).

Copy link

Choose a reason for hiding this comment

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

Ok, that does explain a lot. But using storybook only for tests is a bit of waste I think. It would be great to have option of adding different addons, like np storysource etc from some cli prompt when crating story.

I just integrated storybook in our company repo but went with one storybook per monorepo approach, but I see some benefits from usage per lib.

Have You tried how it works if u try to use affected? Does it run only required storybooks?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

nx affected doesn't work well for the storybook target, because it runs the first one and the process never finishes to run the next one. It would make sense for a storybook-build target though. I haven't written that, but it would be pretty straight forward to add.

nx affected works great for the cypress e2e tests though. For each affected ui-lib, the storybook is launched, the cypress tests run, then all the processes end and the next e2e test target runs.

I would like to include more options for addons, that just hasn't made in this first pass. You'll always be able to go into each .storybook folder and make whatever customizations you want.

@isaacplmann
Copy link
Collaborator Author

Here's a bit more information on what these schematics are intended to generate.

Storybook portion:

  1. .storybook folder inside of a ui lib
  2. *.stories.ts file for each *.component.ts file declared inside that ui lib
  3. A storybook builder target in the angular json for that ui lib that runs storybook (just for that lib)

Cypress integration:

  1. A [ui-lib-name]-e2e app that runs cypress against the storybook for that lib
  2. A *.spec.ts inside the e2e app for each *.component.ts file in the ui lib

@isaacplmann
Copy link
Collaborator Author

isaacplmann commented Jul 15, 2019

The overall goal is to make it quick and easy to run isolated ui tests on individual components using Cypress.

And as a side benefit, you get a storybook instance that you can use for other purposes.

packages/storybook/collection.json Outdated Show resolved Hide resolved
packages/storybook/collection.json Outdated Show resolved Hide resolved
};
}

function updateAngularJsonBuilder(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hm.... I wonder if it would make sense to make devServerTarget and prodDevServerTarget options for @nrwl/cypress:cypress-project

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not in this PR

packages/storybook/src/schematics/component-story/index.ts Outdated Show resolved Hide resolved
packages/storybook/src/schematics/ng-add/ng-add.ts Outdated Show resolved Hide resolved
{},
{
'@storybook/angular': storybookAngularVersion,
'@storybook/addon-knobs': storybookAddonKnobsVersion,
Copy link
Collaborator

Choose a reason for hiding this comment

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

This seems like some plugin... Should we let users add this for themselves if they want to?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm using this plugin for the core functionality of creating isolated cypress ui tests. Eventually, I'd like to give people the choice to include other plugins automatically.

packages/storybook/src/utils/versions.ts Outdated Show resolved Hide resolved
@FrozenPandaz
Copy link
Collaborator

So excited for Storybook integration! 🎉

@vsavkin
Copy link
Member

vsavkin commented Aug 6, 2019

@isaacplmann @FrozenPandaz hey folks, any update on this?

@isaacplmann
Copy link
Collaborator Author

@vsavkin The PR is almost there. I have a e2e test set up, but it keeps silently failing. When I try to manually recreate what the e2e test is doing, everything works as expected.

@@ -1,4 +1,4 @@
export const storybookAngularVersion = '5.1.9';
export const storybookAddonKnobsVersion = '5.1.9';
export const storybookAddonKnobsVersion = '5.0.3';
Copy link
Contributor

Choose a reason for hiding this comment

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

Why can't we use the same version as storybookAngularVersion?
As well, can we update to 5.10.1?

@ThomasBurleson
Copy link
Contributor

ThomasBurleson commented Aug 14, 2019

@isaacplmann - earlier you described the intent of these schematics:

Storybook portion:

.storybook folder inside of a ui lib
*.stories.ts file for each *.component.ts file declared inside that ui lib
A storybook builder target in the angular json for that ui lib that runs storybook (just for that lib)
Cypress integration:

A [ui-lib-name]-e2e app that runs cypress against the storybook for that lib
A *.spec.ts inside the e2e app for each *.component.ts file in the ui lib


I feel there is another important scenario:

The ability to generate a 1...n storybook apps which import storybook Modules for 1...n ui (or feature UI libraries). Having distinct Storybook apps would allow designers to quickly validate UI library components and feature UX.

@Jordan-Hall
Copy link
Contributor

I've been using Storybook for NX for a bit now. Just never got around to writing the schematics. Personally, i feel this PR is too early to be considered complete and the requirements should be scoped out better before rushing into adding new features.

Rather than having multiple .storybook folders which you are required to configure each time. Would it not be better to add a root level folder which contains the global storybook and extend each UI library from the base config. I don't know about you but I also configure storybook with the same plugins

root => .storybook => config.js

import { withKnobs } from '@storybook/addon-knobs';

//import styles
import "`bootstrap";

addDecorator(withKnobs);

root => .storybook => plugins.js import '@storybook/addon-knobs/register';

ui-lib => .storybook => plugin.js import '../../../.storybook/plugins';

ui-lib => .storybook => config.js

import '../../../.storybook/config'

function loadStories() {
  const req = require.context('../', true, /\.stories\.ts$/);
  req.keys().forEach(filename => req(filename));
}


configure(loadStories, module);

@isaacplmann
Copy link
Collaborator Author

@ThomasBurleson
That's a great feature. I think I'll add that as a separate schematic in a future PR.

@Jordan-Hall
I really like that idea. That would make the storybook config more inline with the way cypress, jest, karma, etc are handled.

Thanks to both of you for giving some feedback. I'll put in some more work on this PR tomorrow.

@kbrilla
Copy link

kbrilla commented Aug 15, 2019 via email

@Jordan-Hall
Copy link
Contributor

@isaacplmann I've just noticed we don't have a build option, this PR only has a serve option. It would be good to add support to build the storybook in case we want to deploy it. At least for @ThomasBurleson suggestion of a global storybook. Maybe only offer support to build each lib where a pacakge.json exists.

r = r.filter(rule => rule.test != '/\\.css$/');

// Make whatever fine-grained changes you need
r.push({
Copy link
Contributor

Choose a reason for hiding this comment

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

Think storybook should match every stylesheet nx supports. Also, do we require custom scss support? Under the hood, it uses angular cli which already has it supported

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

There's some conflict between the angular cli and storybook's webpack config that throws displays an error instead of your storybook content unless we do this custom webpack config workaround. See: storybookjs/storybook#3593

I haven't tried stylesheets other than css and scss.

@@ -1,4 +1,5 @@
export const storybookAngularVersion = '5.1.11';
export const storybookAddonKnobsVersion = '5.1.11';
export const storybookAddonKnobsTypesVersion = '5.0.3';
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are default addons required? It's not needed to set it up manually so shouldn't be required to do it programmatically using schematics

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm using the knobs addon in the auto-generated stories. That's probably the first addon most people will include.

Copy link
Member

Choose a reason for hiding this comment

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

Realistically, are there any usages that don't use knobs? I don't think it's a bad default.

@Jordan-Hall
Copy link
Contributor

Just out of curiosity, and i cant wait to see this brought in. Are we not limiting ourselves with this approach to only angular. We also need to look at support react and I think something else needs to be done to get this to work

@isaacplmann
Copy link
Collaborator Author

@Jordan-Hall

Yes, we definitely want to add React support. I haven't started thinking about how to do that though. Any ideas?

@Jordan-Hall
Copy link
Contributor

I've got a few ideas. It appears the only major difference is how you load/configure stories. Maybe for now just adding it as a required field for which framework to support.
import { configure } from '@storybook/react';

along with two extra npm packages react & react-dom

I've currently got your repository checked out looking at a builder for this. I was thinking we could clean up the .storybook folder as long as we pass in the project path.

@isaacplmann
Copy link
Collaborator Author

Nice. I do want to have a builder in place. Right now I'm working on extending a root configuration like you suggested.

@Jordan-Hall
Copy link
Contributor

Which is fine because you may need project-specific configuration which would then require it's own .storybook like your working on now

@@ -0,0 +1,5 @@
{
Copy link
Contributor

Choose a reason for hiding this comment

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

This won't work. It's better not to have this base tsconfig. Your better off enforcing this into each individual app, otherwise, all stories will be included.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is for running a global storybook that will pick up all stories in the monorepo

@@ -2,10 +2,12 @@ import { configure, addDecorator } from '@storybook/angular';
import { withKnobs } from '@storybook/addon-knobs';

function loadStories() {
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 for the global storybook? or is this for each individual to extend, If the answer is for individuals you should have this functionality in here

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is for running a global storybook that will pick up all stories in the monorepo

Copy link
Contributor

Choose a reason for hiding this comment

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

What about extending the base config for the individual one? Wouldn't a better option to add a new application in angular.json for the global storybook to run?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Adding a separate app would work, but the benefit of this .storybook folder configuration is that people who are used to running start-storybook from the command line will get the global storybook

import { configure } from '<%= offsetFromRoot %>../.storybook/config';

function loadStories() {
const req = require.context('../src/lib', true, /\.stories\.ts$/);
Copy link
Contributor

Choose a reason for hiding this comment

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

const req = require.context('../src/', true, /\.stories\.ts$/);
Think this would be better incase someone has an app level storybook

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sure, that's a good idea.

@Jordan-Hall
Copy link
Contributor

@isaacplmann I think I can get a builder sorted for tomorrow night which should work depending on the framework you've selected. Would it be better to wait until after this is merged or should storybook come in as one?

@Jordan-Hall
Copy link
Contributor

@isaacplmann How much longer until this will be ready to be merged in? I've got the basic builder working. Just adding all the other customization to allow more flexibility and then update the schematics and write the unit tests. Shouldn't take too much longer, but should add this is the first time working with the angular devkit.

angular.json

"storybook": {
          "builder": "@nrwl/storybook:storybook",
          "options": {
            "uiFramework": "@storybook/angular",
            "config": {
              "configFolder": "apps/rina-eas/.storybook"
            }
          }
        }

@isaacplmann
Copy link
Collaborator Author

isaacplmann commented Aug 18, 2019 via email

@Jordan-Hall
Copy link
Contributor

Hey,

So I builder has run into an issue when using storybook/angular. storybookjs/storybook#7207 We need to wait until version 5.2 come out. I'll work with 5.2 for now so it's ready on release

@vsavkin vsavkin force-pushed the master branch 2 times, most recently from 3569a53 to 54d5826 Compare August 21, 2019 15:53
@isaacplmann
Copy link
Collaborator Author

@Jordan-Hall , can you share your in progress code? I have more time this week and can help work around the issue you ran into.

@isaacplmann isaacplmann force-pushed the storybook-schematics branch 6 times, most recently from 926327b to 69e4534 Compare October 21, 2019 20:45
@kristofdegrave
Copy link
Contributor

Any idea when this PR will merged and released? I'm looking forward on using this functionality in my projects.

@isaacplmann
Copy link
Collaborator Author

@kristofdegrave, @vsavkin is planning to review it soon, but he just got back from a conference so it might take him a bit.

runCLI(
`generate @nrwl/storybook:configuration ${mylib2} --configureCypress --generateStories --generateCypressSpecs --no-interactive`
);
runYarnInstall();
Copy link
Member

Choose a reason for hiding this comment

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

Could you remove it and instead add storybook dependences to the list of dependencies we copy?

Copy link
Member

Choose a reason for hiding this comment

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

See copyMissingPackages

runCLI(`generate @nrwl/angular:app ${myapp} --no-interactive`);

const mylib = uniq('test-ui-lib');
createTestUILib(mylib);
Copy link
Member

Choose a reason for hiding this comment

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

Since you don't use this library anywhere else, can you move from utils to tihs spec file?

@@ -2,6 +2,7 @@
"fileServerFolder": ".",
"fixturesFolder": "./src/fixtures",
"integrationFolder": "./src/integration",
"modifyObstructiveCode": false,
Copy link
Member

Choose a reason for hiding this comment

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

Why is this necessary?

@vsavkin vsavkin merged commit 4d4ce9f into nrwl:master Oct 23, 2019
@Jordan-Hall
Copy link
Contributor

Did you add schematics for react as the builder supports it

@isaacplmann
Copy link
Collaborator Author

isaacplmann commented Oct 26, 2019 via email

@github-actions
Copy link

This pull request has already been merged/closed. If you experience issues related to these changes, please open a new issue referencing this pull request.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 22, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants