Watch-mode not rebuild dependent packages when package changes

weyert commented 1 month ago

Verify canary release

[X] I verified that the issue exists in the latest Turborepo canary release.

Link to code that reproduces this issue

n/a

What package manager are you using / does the bug impact?

pnpm

What operating system are you using?

Mac

Which canary version will you have in your reproduction?

1.13.4-canary.4

Describe the Bug

If you make a change to a package that an project is depending on the project does not get rebuild

Expected Behavior

I would expect that the project gets rebuild when the package this project depends on get changed

To Reproduce

If you have a project1 that depends on package1, and package2. The project1 has a script defined in its package.json named dev and the package1 and package2 workspace packages have script defined named build.

The dev in turbo.json is defined as:

{
  "pipeline": {
    "build": {
      "dependsOn": [
        "^build"
      ],
      "inputs": [
        "src/**",
        "public/**",
        "test/**",
        "migration/**",
        "patches/**",
        "mocks/**",
        "__mocks__/**",
        // Required to ignore .DS_Store files
        "!dist/**",
        "!lib/**",
        "!coverages/**",
        "!**/.DS_Store"
      ],
      "outputs": [
        "dist/**/*",
        "build/**/*",
        "lib/**/*",
        "typings/**/*",
        ".next/**/*",
        "!.next/cache/**"
      ],
      "outputMode": "new-only"
    },
    "dev": {
      "dependsOn": [
        "^build"
      ],
      "cache": false,
      "persistent": true
    }
  }
}

If I run the command turbo watch dev --filter=project1 all the packages get build and the dev-script of project1 gets started. Now if I make a change to package1 e.g. src/index.ts. The Turborepo daemon detects the following:

2024-05-16T22:03:07.792779Z  WARN turborepo_lib::package_changes_watcher: changed_files: {AnchoredSystemPathBuf("packages/logging/src/ConsoleStructuredLogger.ts")}
2024-05-16T22:03:07.792801Z  WARN turborepo_lib::package_changes_watcher: changed_packages: Ok(Some({WorkspacePackage { name: Other("@company/logging"), path: AnchoredSystemPathBuf("packages/logging") }}))

So it appears that Turborepo has picked up that the package has changed but it doesn't seem to trigger build on the package and then after that completes re-runs the dev-script of project1.

Have I misconfigured my project?

Additional context

No response

anthonyshew commented 1 month ago

I think Nick beat you to it. :smile: https://github.com/vercel/turbo/pull/8161

cc @NicholasLYang to confirm

NicholasLYang commented 1 month ago

Yep! I can do another canary to confirm

weyert commented 1 month ago

@NicholasLYang That would be great!

weyert commented 1 month ago

@NicholasLYang If you can create a canary build that I am happy to test it. I tried to build it myself but I can't seem to get it compiled.

chris-olszewski commented 1 month ago

@weyert 1.13.4-canary.5 has #8161 for testing

weyert commented 1 month ago

Thanks @chris-olszewski going to experiment with it 👯

weyert commented 1 month ago

I have been trying it out when I make a change it seems to partly work for me. If my structure is as follows:

packages/logging
packages/api (depends on `packages/logging`)
projects/app1 (depends on `packages/api` and `packages/logging`)
projects/app2

If I now make a change to packages/logging I can see that rebuilds both the logging and api-packages by running their build-script command, but it doesn't appear to restart the projects/app1 by running its dev-script command when running the watch-mode the following way: ./node_modules/.bin/turbo watch dev --concurrency=25

and having the following turbo.json-file:

{
  "$schema": "https://turborepo.org/schema.json",
  // Environment variables included in globalEnv key will impact the hashes of all tasks.
  "globalPassThroughEnv": [
    // Turborepo
    "TURBO_HASH",
    "TURBO_INVOCATION_DIR",
    "TURBO_EXPERIMENTAL_UI",
    // Node.js
    "NODE_TLS_REJECT_UNAUTHORIZED",
    // removed 300 line for readability
  ],
  "globalEnv": [
    "NODE_ENV",
    "NODE_OPTIONS",
    // removed lines for readability
  ],
  "pipeline": {
    "clean": {
      "cache": false,
      "outputMode": "errors-only"
    },
    "build": {
      "dependsOn": ["^build"],
      "inputs": [
        "src/**",
        "public/**",
        "test/**",
        "migration/**",
        "patches/**",
        "mocks/**",
        "__mocks__/**",
        // Bundled package tarballs
        "bundles/**",
        // Include the typical microservice entrypoint files
        "*.ts",
        "init.ts",
        "bootstrap.ts",
        "app.ts",
        // Include Dockerfiles in the cache invalidation
        "Dockerfile*",
        // Include tsconfig.json changes
        "tsconfig*.json",
        // Required to ignore .DS_Store files
        "!dist/**",
        "!lib/**",
        "!coverages/**",
        "!**/.DS_Store"
      ],
      "outputs": ["dist/**/*", "build/**/*", "lib/**/*", "typings/**/*", ".next/**/*", "!.next/cache/**"],
      "outputMode": "new-only"
    },
    "test": {
      "cache": true,
      "outputMode": "new-only",
      "inputs": [
        "src/**",
        "test/**",
        "migration/**",
        "patches/**",
        "setupTest.ts",
        "setupTest.tsx",
        "global-setup.ts",
        "vitest.config.ts",
        "jest.config.js",
        "jest.config.cjs",
        "jest.config.mjs",
        // Required to ignore .DS_Store files
        "!**/.DS_Store"
      ],
      "outputs": ["coverage/**", "!coverage/code-climate.json"],
      // ^build is generically set here, we haven't fully enumerated which workspaces
      // actually need to build before running tests.
      "dependsOn": ["^build"]
    },
    "lint": {
      "dependsOn": ["^build"],
      "outputs": ["coverage/code-climate.json"],
      "cache": true,
      "outputMode": "full"
    },
    "//start": {
      "cache": false,
      "persistent": true,
      "dependsOn": ["^dev"]
    },
    "dev": {
      "dependsOn": ["^build"],
      "cache": false,
      "persistent": true
    },
    "//#dev:backend": {},
    "//#dev:frontend": {},
    "//#lint": {},
    "//#format:check": {}
  }
}

weyert commented 1 month ago

Do I misunderstand how watch-mode should work for changing dependent packages? Do I need to watch the node_modules myself for changes when using pnpm ?

Also looks like the dev-tasks now get started before all the build-tasks have been completed causing projects not able to start because it will rely on missing dist/build artifacts.

NicholasLYang commented 1 month ago

With persistent tasks, we don't restart them unless the entire workspace graph is invalidated (i.e. a turbo.json is changed or new dependency is added). We generally assume that persistent tasks are self healing, so even if they fail in the first run, eventually the build-tasks will produce the artifacts and the dev-tasks will start working. Is that not the case with your setup? If so, that's very helpful to know.

We don't restart persistent tasks because Turborepo can't tell if they've completed, so we don't know if we're restarting in the middle of a build. But perhaps we can adjust that logic if restarting persistent tasks turns out to be useful.

weyert commented 1 month ago

Yeah, I am currently having a import-loader defined to accept Typescript code and then use node's --watch functionality to detect changes but they ignore changes in node_modules.

I am wondering what would be the best approach is. What about Turborepo sends the SIGUSR2 signal to the persistent task and then it can decided to handle it or not?

Maybe opt-in flag to indicate it can restarted (debounced) when its dependencies/packages change? nx appears to allow to pass arbitrary command, e.g. turbo run dev, e.g. turbo watch build -- turbo run dev, see more info at: https://www.jvandemo.com/how-to-execute-a-command-every-time-an-nx-project-changes/ and https://nx.dev/nx-api/nx/documents/watch?ref=jvandemo.com

npx nx watch --all -- node your-script.js

# Watch all packages named `-service` and run `node -r @swc-node/register ./bootstrap.ts` when a change has occurred in the project or its dependencies
pnpm exec turbo watch --filter={services\*-service} -- node -r @swc-node/register ./bootstrap.ts

Maybe @gajus has ideas, the creator of turbowatch :)

I do wonder if I could listen to the changes of daemon and somehow handle the case of running a command like the above node -r @swc-node/register ./bootstrap.ts by myself by listening to the daemon ipc protocol?

gajus commented 1 month ago

With persistent tasks, we don't restart them unless the entire workspace graph is invalidated (i.e. a turbo.json is changed or new dependency is added). We generally assume that persistent tasks are self healing,

We've made the same assumptions in our Turbowatch setup.

gajus commented 1 month ago

Yeah, I am currently having a import-loader defined to accept Typescript code and then use node's --watch functionality to detect changes but they ignore changes in node_modules.

We opted not to use node --watch for this reason, and just kill/restart the process.

NicholasLYang commented 1 month ago

In this case I'd agree with @gajus and recommend that not using node --watch is ideal. There's a subtle distinction that needs to be made between something like next dev and node --watch. With the former, it's running a dev server which efficiently rebuilds the app. This is materially different from running next build with turbo watch, since the dev server is way faster. Whereas with node --watch, it's essentially just doing what turbo watch is doing already, which is watching files and re-executing a node process. So in that case it makes sense to strip out the node --watch and directly execute the process, while letting turbo watch handle the watching. At least, I think that's the case. Feel free to correct me if node --watch provides a specific benefit to your workflow!

weyert commented 1 month ago

Yes, but turbo doesn't kill and restart the process when it's a persistent task and that is required when you want to keep a server running. I would need to implement wrapper and again myself watch for changes which doesn't seem efficient as you also mentioned.

Especially, because turbo already knows what has changed and whether the made change affects the persistent task. I think it would be nice to be able to hook into that when I need to write a wrapper which starts a subprocess. If I would be able tap into then I could avoid the issues, such as EMFILE: too many open files.

I will experiment a bit more on my end :)

weyert commented 1 month ago

@NicholasLYang Is this IPC protocol for daemon documented somewhere?

gajus commented 1 month ago

Yes, but turbo doesn't kill and restart the process when it's a persistent task [..]

This is why in turbowatch there is persistent configuration, and also interruptible.

When interruptible is set to true, then turbowatch kills the process when it detects file changes.

weyert commented 1 month ago

I wished I wouldn't need to dependent on turbowatch or extraneous file watching to achieve this. Now I will be watching the same things twice. Once by turbo watch and once by myself or turbowatch for each of the persistent task defined in turbo.

The problem I am having is that I then get all kind of issues when I try to watch my 15 persistent tasks at the same time; as I am running into the problem that I hit the maximum of allowed opened files in macOs. I already had to increase it to 16.777.216

gajus commented 1 month ago

The problem I am having is that I then get all kind of issues when I try to watch my 15 persistent tasks at the same time; as I am running into the problem that I hit the maximum of allowed opened files in macOs. I already had to increase it to 16.777.216

You should not run into max open file issues on latest Node.js versions. Any version v19.1. See notes

NicholasLYang commented 1 month ago

@NicholasLYang Is this IPC protocol for daemon documented somewhere?

There's a protobuf file, but no official documentation.

Yes, but turbo doesn't kill and restart the process when it's a persistent task and that is required when you want to keep a server running.

Ah, so to confirm, you can't strip out the --watch and rely on turbo watch with a non-persistent version of the task because you want a dev server to keep running? That makes sense. I can bring up with the team the possibility of making persistent tasks interruptible.

weyert commented 1 month ago

Yes, I have 14 backend servers, one Rust service, and two ASP.net services, and 4 Next.js projects plus ton of packages adding up to around 80+ packages in total. I would like to be able to watch roughly 9 backend servers and have them be restarted/reloaded when I change its code or any of the dependencies.

anthonyshew commented 3 weeks ago

Checking back in here. Are we able to close this one out? Judging by the title and if memory serves me correctly, I think this was from a canary release that is now fixed?