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

Build: Add NX bootstrap optimization #14535

Merged
merged 5 commits into from
Apr 27, 2021

Conversation

FrozenPandaz
Copy link
Contributor

@FrozenPandaz FrozenPandaz commented Apr 9, 2021

What I did

This PR adds Nx to run the prepare stage. Nx has remote caching which would allow developers to reuse builds done on the CI server. This will make getting started with the repo as well as CI faster.

For more information about Nx, see the official documentation

How to test

Running nx run-many --target prepare --parallel --all should successfully build all projects.

Closes #14504

@nx-cloud
Copy link

nx-cloud bot commented Apr 12, 2021

Nx Cloud Report

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

📂 See all runs for this branch

Status Command
#000000 nx run-many --target=prepare --all --parallel --max-parallel=15

Sent with 💌 from NxCloud.

@FrozenPandaz FrozenPandaz changed the base branch from tech/nx-tooling to next April 13, 2021 00:02
@ndelangen
Copy link
Member

@FrozenPandaz do you want to pair again to try and get this across the line & merged?

@ndelangen ndelangen added the maintenance User-facing maintenance tasks label Apr 21, 2021
@ndelangen ndelangen self-assigned this Apr 21, 2021
@ndelangen
Copy link
Member

Super excited about this!

@@ -81,7 +81,7 @@
"@angular/forms": "^11.2.0",
"@angular/platform-browser": "^11.2.0",
"@angular/platform-browser-dynamic": "^11.2.0",
"@nrwl/workspace": "^11.1.5",
"@nrwl/workspace": "^11.6.3",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is an update to the devDependency.

This new version has some allowances that allow Nx to be tested within an Nx repo.

@@ -95,8 +95,8 @@ function run() {
command: () => {
log.info(prefix, 'prepare');
spawn(
`lerna run prepare ${
process.env.CI ? `--concurrency ${maxConcurrentTasks} --stream` : ''
`nx run-many --target=prepare --all --parallel ${
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This changes the command to use Nx instead of lerna which has remote caching!

@@ -9,9 +9,13 @@ import { executor as managerExecutor } from './manager/builder';
import { buildDevStandalone } from './build-dev';
import { buildStaticStandalone } from './build-static';

// nx-ignore-next-line
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These were some imports that created circular dependencies. These comments ignore the dependencies generated by these lines.

@@ -24,6 +24,7 @@ export const readAngularWorkspaceConfig = async (

const nxWorkspace = require('@nrwl/workspace').readWorkspaceConfig({
format: 'angularCli',
path: dirToSearch,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now that this method is used within a Nx workspace, it began to resolve the workspace.json at the root of this repo in unit tests.

This should not affect end users of Storybook. This is a fix for some tests that use this method.

@ndelangen
Copy link
Member

@FrozenPandaz I think there's a bug in the NX integration.. When I click on "All runs for this branch" I see this url:
https://nx.app/branch?workspaceId=606dc2e0c2e8d5671d813305&branchName=14535

I think the branch-name is wrong. That looks like the PR-number to me.

Comment on lines 13 to +18
import reactOptions from '../../../app/react/src/server/options';
// nx-ignore-next-line
import vue3Options from '../../../app/vue3/src/server/options';
// nx-ignore-next-line
import htmlOptions from '../../../app/html/src/server/options';
// nx-ignore-next-line
Copy link
Member

Choose a reason for hiding this comment

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

@shilman If we wanted to remove these circular dependencies we could extract this into it's own util, or duplicate the code (wouldn't recommend that)

@FrozenPandaz
Copy link
Contributor Author

I think the branch-name is wrong. That looks like the PR-number to me.

Yes, you are right, that is the PR number.

The PR number is automatically picked up from circle.

We will make it less confusing. 👍

@shilman shilman changed the title chore: update nx Build: Add NX bootstrap optimization Apr 27, 2021
Copy link
Member

@shilman shilman left a comment

Choose a reason for hiding this comment

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

This is amazing 🤯 Thank you @FrozenPandaz !!!

@shilman shilman merged commit eedf6c7 into storybookjs:next Apr 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintenance User-facing maintenance tasks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants