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

chore: add nx #11461

Closed
wants to merge 1 commit into from
Closed

chore: add nx #11461

wants to merge 1 commit into from

Conversation

jaysoo
Copy link

@jaysoo jaysoo commented Dec 21, 2022

Description

This is an example PR of adding Nx to the Vite repo. Myself and Juri discussed with @patak-dev about whether this repo could benefit from Nx so I'm outlining the changes and benefits here.

Note: I added Nx Cloud access token to nx.json. This way contributors can use it as well, but you can also remove it and set NX_CLOUD_ACCESS_TOKEN in CI, and provide a read-only token for contributors (or remove it entirely).

Additional context

This main monorepo does not have many packages left, and create-vite I heard is also moving to a separate repo later. However, I can still see two main benefits for using Nx:

  1. Setting up tasks and its dependencies and letting Nx handle execution + visualizing the task graph.
  2. Using caching to skip necessary tasks locally and in CI.

Basically, it doesn't matter if this repo is a monorepo of not, Nx is used as a task runner and has sophisticated distributed caching support.

Task execution and caching through Nx unlocks many benefits, such as:

  • Easier method of configuring and debugging task dependencies
  • Contributors to docs will have a better first experience
  • Faster CI checks when updating docs
  • Faster CI checks for @vitejs/plugin-legacy fixes
  • Faster CI checks when pushing multiple times to the same PR

I'll go into the above points into more details next. This PR only provides basic configuration changes, but there are more optimizations that are possible if desired.

Easier method of configuring and debugging task dependencies

There are tasks that implicitly depend on other tasks.

e.g.

  • test -> vite:build
  • test-docs -> docs-build -> vite:build
  • test-build -> vite:build

Using Nx, we can explicitly define these dependencies in nx.json. There is a lot more control afforded here than through ad-hoc package scripts.

e.g.

"test-unit": {
  "dependsOn": ["^build"]
},
"test-build": {
  "dependsOn": ["^build"]
},

The above tells Nx that test-unit and test-build both depend on build of their dependencies (the ^ is a shorthand for dependencies).

Note: You can also use the long-form: { "projects": "dependencies", "target": "build" }.

Nx provides task visualization through the nx graph command. Here are some examples.

Build task graph:
build-task-graph

Docs task graph:
docs-task-graph

Using the visualizations, you can debug issues where task dependencies are set up incorrectly -- maybe dependencies are missing, or shouldn't exist, the graph will show that.


Contributors to docs will have a better first experience

If a contributor only changes files in docs/ folder, then running the following command should be sufficient, and should start the server as fast as possible (almost instantly).

pnpm install
pnpm run docs

To achieve this experience, we configure nx.json to add inputs for the different tasks.

  1. Added two named inputs default and production. Default inputs are all files under packages/ folder, whereas production inputs are files that affect the published npm package.
  2. Configured docs task to only be affected by files under docs/, and depend on the build task.
  3. Configured build task to only be affected by production files (e.g. changes to spec files do not affect build).
"namedInputs": {
  "default": ["{workspaceRoot}/packages/**/*"],
  "production": [
    "default",
    "!{workspaceRoot}/packages/**/?(*.)+(spec|test).[jt]s?(x)?(.snap)",
    "!{workspaceRoot}/vitest.config.e2e.ts",
    "!{workspaceRoot}/vitest.config.ts",
    "!{workspaceRoot}/.eslintignore",
    "!{workspaceRoot}/.eslintrc.cjs",
    "!{projectRoot}/tsconfig.check.json"
  ],
},
"targetDefaults": {
  "docs": {
    "dependsOn": ["^build"],
    "inputs": ["{workspaceRoot}/docs/**/*"]
  },
  "build": {
    "dependsOn": ["^build"],
    "inputs": ["production"],
    "outputs": ["{projectRoot}/dist"]
  },
},

With the above configuration, contributors can get started with just two commands:

pnpm install
pnpm run docs

And since build is not affected by docs changes, the server should start almost immediately (~5s for my locally). The build cache is pulled from Nx Cloud so user never needs to build locally.

See here for more info on inputs: https://nx.dev/more-concepts/customizing-inputs#customizing-inputs-and-named-inputs


Faster CI checks when updating docs

Similar to the previous section, when files in docs/ are changed (and nothing else), then most of the CI checks do not need to run, or at least they can be cached.

This means the time it takes to update docs and getting it to production should be decreased significantly.

Some examples of docs-only PRs:

  • docs: update extensions generated in library mode
    • CI duration: 8m 46s
    • Cacheable operations: typecheck, lint, build, test-unit, test-serve, test-build
    • Should be passing checks in < 3m (just setup and running test-docs)
  • docs: announcing Vite 4 post
    • Changes are only in docs
    • CI duration: 12m 27s
    • Cacheable operations: typecheck, lint, build, test-unit, test-serve, test-build
    • Should be passing checks in < 3m (just setup and running test-docs)

Faster CI checks for @vitejs/plugin-legacy fixes

For changes in only @vitejs/plugin-legacy package, there is no need to run tasks relating to vite nor docs.

  • fix: 修复现代浏览器不支持 catch optional
    • Changes are only in plugin-legacy
    • CI duration: 11m 23s
    • Cacheable operations: typecheck, vite:build, test-unit, test-docs
    • Looks like only lint and @vitejs/plugin-legacy:build, test-serve, and test-build are needed to run (both are ~10s)
    • Should be passing checks in < 10m (can be even faster with more tweaks to test-build and test-serve, e.g. separate tests for legacy plugin vs main vite packages)

Faster CI checks when pushing multiple times to the same PR

When tasks hve the correct inputs configured, then Nx will re-run only what's necessary.

For example, if developer pushes changes to production and spec files, the first run will need to re-build and re-test everything.

However, if a second push only changes a spec file, then builds and docs are skipped.


What is the purpose of this pull request?

  • Bug fix
  • New Feature
  • Documentation update
  • Other

Before submitting the PR, please make sure you do the following

  • Read the Contributing Guidelines.
  • Read the Pull Request Guidelines and follow the PR Title Convention.
  • Check that there isn't already a PR that solves the problem the same way to avoid creating a duplicate.
  • Provide a description in this PR that addresses what the PR is solving, or reference the issue that it solves (e.g. fixes #123).
  • Ideally, include relevant tests that fail without this PR but pass with it.

}
}
},
"namedInputs": {
Copy link
Author

Choose a reason for hiding this comment

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

See description for how inputs and namedInputs can help local dev and CI experience.

"playground": [
"default",
"{workspaceRoot}/playground/**/*",
{
Copy link
Author

Choose a reason for hiding this comment

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

inputs can include environment variables or the result of a shell script.

Copy link
Contributor

Choose a reason for hiding this comment

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

would it also include extra cli arguments? eg pnpm test-serve some-test-file-to-focus.ts

Copy link
Author

Choose a reason for hiding this comment

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

yeah, extra args should be included already

}
]
},
"targetDefaults": {
Copy link
Author

Choose a reason for hiding this comment

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

This section configures tasks and their dependencies + inputs that affect their computation.

@@ -19,17 +19,17 @@
"lint": "eslint --cache .",
"typecheck": "tsc -p scripts --noEmit && pnpm -r --parallel run typecheck",
"test": "run-s test-unit test-serve test-build",
"test-serve": "vitest run -c vitest.config.e2e.ts",
"test-build": "VITE_TEST_BUILD=1 vitest run -c vitest.config.e2e.ts",
"test-serve": "nx exec -- vitest run -c vitest.config.e2e.ts",
Copy link
Author

Choose a reason for hiding this comment

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

nx exec is used to wrap any package scripts that that should run through Nx. This way non of the local or CI scripts need to be updated to integrate with Nx.

For example, users continue to use pnpm run docs not nx run docs.

Copy link
Contributor

Choose a reason for hiding this comment

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

does the nx cli prefix/modify log output of the original command?
does it allow passing additional args like focussing a test?

Choose a reason for hiding this comment

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

the nx exec command executes whatever command you pass to it (including any flags). The output is rendered with the exact same colors and formatting. The only modification is that Nx adds a message at the end saying "replayed from cache" or something similar.

@jaysoo jaysoo force-pushed the chore/add-nx branch 4 times, most recently from acbe9df to 9b9d746 Compare December 21, 2022 17:49
@patak-dev
Copy link
Member

Thanks a lot for the PR and such a detailed explanation of the benefits @jaysoo!
I'm going to bring this proposal to the next team meeting to discuss with others. Personally, I think this could be good for core dev and I hope we can take advantage of it.
Just to set expectations, the next meeting will happen in three weeks because several team members took the last two weeks of December as a vacation. I think it is good though because we need some time to review the implications with others in the ecosystem.

Two things I would like we evaluate:

  • We're planning to add codeflow to the repo. I will forward this PR to the StackBlitz team in the first week of January so we can review how it would affect the PR review process. I think it would be ok though because we can always run the scripts directly in WebContainers and avoid caching if we think that gives a better experience.
  • We need to review with @dominikg how this would affect https://github.com/vitejs/vite-ecosystem-ci. I think there is a chance that this would be quite beneficial for ecosystem CI because we could avoid the need to build Vite 15+ times for each run, and get it from the cache after the first build.

Again, thanks so much for taking the time and send us such a careful evaluation of what it would mean to use Nx in Vite Core.

@patak-dev patak-dev added the p3-significant High priority enhancement (priority) label Dec 21, 2022
@benmccann
Copy link
Collaborator

I think it's great that Nx and Vite are working together and I'm appreciative of the efforts made here.

We've setup a tool called Turbo (https://turbo.build/) for SvelteKit. It looks like Nx is very similar and in fact the config files look almost identical. I don't mean to be a downer here especially since I have no direct experience with Nx, but I have to be very honest that Turbo is the single most upsetting piece of technology I've ever dealt with and I can't wait for it to vanish from this earth. Its configuration is extremely brittle. It's a constant struggle that we've accidentally missed configuring some piece of input or output and Turbo returns a cached version of our job instead of executing it. Caching is one of the hard things in computer science and I've spent way more time dealing with getting it right than Turbo has ever saved me. While in some ways it's not fair to judge Nx based on my experience with Turbo, the similarities between the tools are so striking it's hard for me not to imagine that Nx would be prone to the same issues.

I'm very very very strongly against this change. Kill it with fire. The extra layer of potential headaches is not worth the time savings in my opinion.

I really do hate to leave such a negative comment on your first PR in this repo, but would feel remiss if I didn't share my experience. I do thank you for your efforts here and welcome you to the Vite community and hope we will see more PRs from you in the future

@dominikg
Copy link
Contributor

dominikg commented Dec 26, 2022

I share Ben's sentiments, the complexity and risk of adding a caching layer outweigh any benefits it may bring.
We not only add new dev-time dependencies, we also depend on additional infrastructure. I'm sure you're doing your best to keep nx cloud up and humming, but it is an extra thing that can break.
It could also be attacked, trying to feed us and our contributors/users poisoned caches. So the approach in this PR with a public token is not going to work at all.
We need absolute guarantees that it is impossible for these caches to be tampered with - either by contributors or by nx cloud admins - to even consider using them - at all. I'd go as far and say that release builds must never use any.

Even if we get beyond all this and it somehow always works and was low maintenance, it would still be doing extra things to solve problems that are not really problems in this repo and even if they become problems, we have other options to deal with them:

  1. visualization of task dependencies.
    There are only very few tasks in this repo and there are no deep dependencies between them. Once more plugins move out, this is going to be even less of a concern. If something is unclear, a simple paragraph in the contributors guide would help clear that up, no extra bin needed.

  2. cached builds speeding things up.
    Vite-4 already reduced the build time and we can do more to reduce it even further.

  • Move the remaining plugins out
  • reduce time it takes to generate types (and skip where not needed)
  • Use esbuild and maybe even stc for a considerable speed gain
    pnpm build takes around 30s on my machine rn, with the changes above it would probably drop below 10s.

Ultimately it is even possible to not need build for some or all things. Either by using a typescript aware node loader or by having the source in .js files with types in jsdoc (this works great for sveltekit, but would be a huge undertaking for vite, don't expect that to happen anytime soon or at all, but it is possible)

so we can get builds to plenty fast without needing a cache. Even the current build speeds havn't raised concerns to my knowledge.

  1. skipping unneeded tests/tasks in CI
    This is a great idea, but instead of relying on a complicated extra config and binary, we should just not do them in the first place. Doing things only once with a cache is nice, but not doing things that aren't needed is nicer.

Don't get me wrong, nx is an invaluable tool for larger monorepos and organizations that can make the most of it's featureset in a controlled environment, but for the vite monorepo, i don't think adding it would be beneficial right now.

"test-docs",
"docs-build"
],
"accessToken": "NjYwZGFlNWYtZGYyNS00MTIzLTkzYTEtYzAxZTZiYzUwYTVifHJlYWQtd3JpdGU="
Copy link
Contributor

Choose a reason for hiding this comment

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

as mentioned in my general comment, this key must not provide write access to the cache or it would be far too dangerous to use.

Choose a reason for hiding this comment

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

Correct. The accessToken that is committed to an open source repo should always be a read only token. I'm sure @jaysoo set this token to read only. You can use a read-write token in a .env file that is not committed to the repo on trusted dev machines or only use the read-write token in the environment variables in CI.

@dominikg
Copy link
Contributor

Is there any form of data collection or tracking in the nx cli or nx cloud?

@jaysoo
Copy link
Author

jaysoo commented Jan 9, 2023

Is there any form of data collection or tracking in the nx cli or nx cloud?

No, the CLI does not have any telemetry. Nx Cloud collects historical data for scheduling purposes (average task runs are used to divvy out tasks to agents if Distributed Task Execution is enabled).

@jaysoo jaysoo closed this Jan 18, 2023
@jaysoo
Copy link
Author

jaysoo commented Jan 18, 2023

Closing this PR since the improvement is not worth the effort at this point. We can revisit in the future, even for another repo. Always open to support.

@patak-dev
Copy link
Member

Thanks for taking the time to show us how Nx would work in Vite core @jaysoo. We really appreciate it. For others reading, we discussed with the team and we found the benefits compelling but decided to delay adopting another tool until CI and build times start to be an issue for us. We'll re-evaluate in the future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p3-significant High priority enhancement (priority)
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

5 participants