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): allow to specify angular target project for storybook #4099

Merged
merged 3 commits into from
Jan 13, 2021

Conversation

hiepxanh
Copy link
Contributor

@hiepxanh hiepxanh commented Nov 17, 2020

feat(storybook): adding enviroment variable which allow storybook get angular project target

Since this PR storybookjs/storybook#12565 already merge, and going to release with storybook 6.1.
As he said: "Basically, we now verify if there is an environment variable called "STORYBOOK_ANGULAR_PROJECT". If so, we use its value as the project name."
This PR going to add an enviroment variable call "STORYBOOK_ANGULAR_PROJECT" and the storybook going to catch that enviroment variable. Then it can recognize the project name correctly.
This PR only to work with the new storybook version, so why we not release the update first, then after they release it. Every body automatic got this feature :D voila

Current Behavior

image

Using "default" project from angular.json to run storybook

Expected Behavior

Using project target while using storybook

Related Issue(s)

Fixes #3655 #4305

@nx-cloud
Copy link

nx-cloud bot commented Nov 17, 2020

Nx Cloud Report

CI ran the following commands for commit 28823c6. Click to see the status, the terminal output, and the build insights.

Status Command Start Time
#000000 nx build-base angular 1/13/2021, 2:41:34 PM
#000000 nx build-base create-nx-plugin 1/13/2021, 2:41:42 PM
#000000 nx build-base create-nx-workspace 1/13/2021, 2:41:28 PM
#000000 nx build-base cypress 1/13/2021, 2:41:21 PM
#000000 nx build-base dep-graph-client --configuration release 1/13/2021, 2:40:21 PM
#000000 nx build-base eslint-plugin-nx 1/13/2021, 2:41:28 PM
#000000 nx build-base express 1/13/2021, 2:41:51 PM
#000000 nx build-base jest 1/13/2021, 2:41:21 PM
#000000 nx build-base linter 1/13/2021, 2:41:21 PM
#000000 nx build-base nest 1/13/2021, 2:41:51 PM
#000000 nx build-base next 1/13/2021, 2:41:57 PM
#000000 nx build-base node 1/13/2021, 2:41:34 PM
#000000 nx build-base nx-plugin 1/13/2021, 2:41:27 PM
#000000 nx build-base react 1/13/2021, 2:41:51 PM
#000000 nx build-base storybook 1/13/2021, 2:41:41 PM
#000000 nx build-base web 1/13/2021, 2:41:34 PM
#000000 nx run-many --target=build --all --parallel 1/13/2021, 2:40:00 PM
#000000 nx run-many --target=e2e --projects=e2e-storybook 1/13/2021, 2:40:34 PM
#000000 nx run-many --target=lint --all --parallel 1/13/2021, 2:42:05 PM
#000000 nx run-many --target=test --all --parallel 1/13/2021, 2:37:46 PM

Sent with 💌 from NxCloud.

@hiepxanh
Copy link
Contributor Author

hiepxanh commented Nov 17, 2020

Sad, why I just add only 1 enviroment variable but it can make the test failing?

@hiepxanh
Copy link
Contributor Author

@juristr hello sir, can you help me to verify the new code? storybook 6.1 has been release and it should work with my new code, but I dont understand, why it fail

@juristr
Copy link
Member

juristr commented Nov 21, 2020

Hey thx for the contribution. I'll give it a look next week

@juristr
Copy link
Member

juristr commented Nov 27, 2020

@hiepxanh the problem why the build fails is due to the git commit message check:

image

Can you squash your two commits and use this message:

feat(storybook): adding environment variable for storybook to find correct angular project target

Thx :)

@hiepxanh
Copy link
Contributor Author

@juristr yes sir, I already pushed new commit

@hiepxanh
Copy link
Contributor Author

@juristr I already follow the instruction, why it still fail sir?
image

@hiepxanh
Copy link
Contributor Author

@juristr my yarn verified it
image

@juristr
Copy link
Member

juristr commented Nov 30, 2020

@hiepxanh they aren't failing due to the commit message this time. There was some error in the pipeline, might have been a CircleCI issue. I'll restart them and then we'll see.

@hiepxanh
Copy link
Contributor Author

thank you sir

@juristr
Copy link
Member

juristr commented Nov 30, 2020

@hiepxanh looks like something went wrong with your original rebase. Anyway, I fixed it and pushed again to your branch. The PR looks good to me. I'll give it a test run and then merge it.

I just want to get the upgrade to Storybook v6.1 in first as well before I merge this one.

Thx for your contribution :)

@hiepxanh
Copy link
Contributor Author

hiepxanh commented Dec 1, 2020

wow! thank you so much sir, sorry for my clumsy

@hiepxanh
Copy link
Contributor Author

@juristr dont you mind if you take a look with my PR?

@juristr
Copy link
Member

juristr commented Dec 11, 2020

I did, but I have to think this through more deeply. Your PR works on apps, however needs some improvements when you run storybook in libs.

Right now we're busy on preparing the Nx 11 release. I'll take a closer look immediately after that

@hiepxanh
Copy link
Contributor Author

:D resolved conflict

@juristr
Copy link
Member

juristr commented Jan 8, 2021

@hiepxanh I'm looking at your PR right now. The main issue here is that in this way when you generate a storybook config for an Nx library, the according "build" won't be found, thus you get something like

ERR! architect.build is not defined for project 'ui'.

(where ui is my Nx lib). Because now the library config in workspace.json is being used which doesn't have a build config. Which might be fine, but changes the behavior compared to before, where some of the app's configs in the workspace would have been used.

I'm wondering whether we should give the possibility to specify a property in the storybook config in the workspace.json that allows to point to an app that should be used as the config 🤔 . I'm going to give that a quick try tomorrow and push a fix in case s.t. we can get this merged

@hiepxanh
Copy link
Contributor Author

hiepxanh commented Jan 8, 2021

thank you, that is a greate idea,I think your solution should be fine 😄

@juristr
Copy link
Member

juristr commented Jan 9, 2021

@hiepxanh here's the solution I came up with.

a) "app" type projects will always get the context.target.project as that's the only thing that makes sense.

b) if "lib" type projects are being started with storybook, then the normal storybook app resolution will take place, unless you specify a different app project in the corresponding configuration in the "angular.json" (or workspace.json).
Say you have a lib ui, then the storybook config could look like

"storybook": {
  "builder": "@nrwl/storybook:storybook",
  "options": {
    "uiFramework": "@storybook/angular",
    "port": 4400,
    "appBuildConfig": "testapp", // <<<<<<<<<
    "config": {
      "configFolder": "libs/ui/.storybook"
    }
  },
  "configurations": {
    "ci": {
      "quiet": true
    }
  }
},

By adding appBuildConfig to testapp, then Storybook will use the testapp as the Angular app to fetch the config from.

What do you think? Also, if you have a better name in mind than appBuildConfig let me know 😅. It works locally, going to wrap it up tomorrow or latest Monday. I'll then push it up onto your branch and we merge this PR in the end, ok? Let me know what you think

@hiepxanh
Copy link
Contributor Author

Yes, that exactly what I'm thinking of. I belive the appBuildConfig name should be projectBuildConfig since that you can point to an application or a library, for example:

  • projectBuildConfig: nx-airplane
  • projectBuildConfig: nx-airplane-ui
    that is look perfect :D

Allows to specify the `projectBuildConfig`
on the storybook config in the
workspace.json
@juristr juristr changed the title feat(storybook): adding enviroment variable which allow storybook get angular project target feat(storybook): allow to specify angular target project for storybook Jan 13, 2021
@juristr juristr merged commit 504466f into nrwl:master Jan 13, 2021
@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 21, 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.

@nrwl/storybook Set Project/App to take build config
2 participants