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

Watch-mode not rebuild dependent packages when package changes #8164

Closed
1 task done
weyert opened this issue May 16, 2024 · 26 comments · Fixed by #9228
Closed
1 task done

Watch-mode not rebuild dependent packages when package changes #8164

weyert opened this issue May 16, 2024 · 26 comments · Fixed by #9228
Labels
kind: bug Something isn't working

Comments

@weyert
Copy link
Contributor

weyert commented May 16, 2024

Verify canary release

  • 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

@weyert weyert added kind: bug Something isn't working needs: triage New issues get this label. Remove it after triage owned-by: turborepo labels May 16, 2024
@anthonyshew
Copy link
Contributor

I think Nick beat you to it. 😄
#8161

cc @NicholasLYang to confirm

@NicholasLYang
Copy link
Contributor

Yep! I can do another canary to confirm

@weyert
Copy link
Contributor Author

weyert commented May 17, 2024

@NicholasLYang That would be great!

@anthonyshew anthonyshew removed the needs: triage New issues get this label. Remove it after triage label May 20, 2024
@weyert
Copy link
Contributor Author

weyert commented May 20, 2024

@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
Copy link
Member

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

@weyert
Copy link
Contributor Author

weyert commented May 20, 2024

Thanks @chris-olszewski going to experiment with it 👯

@weyert
Copy link
Contributor Author

weyert commented May 20, 2024

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
Copy link
Contributor Author

weyert commented May 20, 2024

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
Copy link
Contributor

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
Copy link
Contributor Author

weyert commented May 21, 2024

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
Copy link

gajus commented May 21, 2024

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
Copy link

gajus commented May 21, 2024

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
Copy link
Contributor

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
Copy link
Contributor Author

weyert commented May 22, 2024

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
Copy link
Contributor Author

weyert commented May 22, 2024

@NicholasLYang Is this IPC protocol for daemon documented somewhere?

@gajus
Copy link

gajus commented May 22, 2024

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
Copy link
Contributor Author

weyert commented May 22, 2024

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
Copy link

gajus commented May 22, 2024

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
Copy link
Contributor

@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
Copy link
Contributor Author

weyert commented May 22, 2024

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
Copy link
Contributor

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?

@orjan
Copy link

orjan commented Jun 8, 2024

#8317 (comment)

This issue was mentioned about regarding “interruptable” tasks but it might be better to create a new issue?

@pieterjandebruyne
Copy link

#8434 might also be related

@marklai1998
Copy link

marklai1998 commented Jul 29, 2024

Recently hop in the turborepo train and trying to improve the local dev DX, I can't believe it's not a thing.
Now I've to restart my multi backend setup every time I make a change to a shared package

I've watch through all the links, and tried most of the possible config combination it still doesn't work.
The issue is that you need "persistent: true" for the watch to work but it doesn't reload persistent task itself

Shared lib are built and store in other services node_module, unless you have things like nodemon that explicitly watch the node_modele, the backend just never restart.

I understand that restarting is slower, but it only hold true for the frontend land where you have heavy start like webpack, backend on the other hand we don't have these luxury build step. Usually only have tsup/tsc/nest-cli(if you use nestjs), that doens't watch for node_modeule. Not to mention turbo knows way better of what have changed compare to nodemon blindly restart every time.
Also the idea of hot reload in FE doesn't apply to BE, instead of replacing the code seamlessly, we need our code to completely re-run everytime it changes

I think “interruptable” is the way to go to make persistent task auto restart-able

@gajus
Copy link

gajus commented Jul 29, 2024

Recently hop in the turborepo train and trying to improve the local dev DX, I can't believe it's not a thing. Now I've to restart by multi backend setup every time I make a change to a shared package

I've watch through all the links, and tried most of the possible config combination it still doesn't work. The issue is that you need "persistent: true" for the watch to work but it doesn't reload persistent task itself

Shared lib are built and store in other services node_module, unless you have things like nodemon that explicitly watch the node_modele, the backend just never restart.

I understand that restarting is slower, but it only hold true for the frontend land where you have heavy start like webpack, backend on the other hand we don't have these luxury build step. Usually only have tsup/tsc/nest-cli(if you use nestjs), that doens't watch for node_modeule. Not to mention turbo knows way better of what have changed compare to nodemon blindly restart every time. Also the idea of hot reload in FE doesn't apply to BE, instead of replacing the code seamlessly, we need our code to completely re-run everytime it changes

I think “interruptable” is the way to go to make persistent task auto restart-able

You can still use turbowatch for this use case.

We never migrated to turbo's native watch for the same reason.

@Tofandel
Copy link

Tofandel commented Oct 4, 2024

I'm trying to have turbo run a long lived dev php server outside of the project in watch mode

    "//#run:server": {
      "dependsOn": [
        "protos#build"
      ],
      "cache": false,
      "interactive": false,
      "inputs": ["../src/**/*.php"]
    },

But yeah it doesn't restart the server when I change anything in src, I'm not sure it's even watching there (I do know it watches correctly in non watch mode)

Any progress on this?

NicholasLYang added a commit that referenced this issue Oct 15, 2024
### Description

Adds the ability to annotate persistent tasks as interruptible. Fixes
#8164.

NOTE: This does create change in behavior for watch mode where new
changes will *stop* the current executing and start a new one.
Previously, we waited for runs to finish, but now that persistent tasks
can be interrupted, we can't do that since persistent tasks don't
finish.

### Testing Instructions

Added tests for ensuring that `interruptible` requires `persistent`, but
otherwise you'll have to try this out manually since it's watch mode.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind: bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants