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

fix(node): use proper watch value with the node:execute executor #4083

Merged
merged 9 commits into from
Jan 8, 2021

Conversation

CurtisHughes
Copy link
Contributor

@CurtisHughes CurtisHughes commented Nov 13, 2020

Current Behavior

When running a node app, the execute builder continues to watch for file changes when the watch mode is set to false.

"serve": {
    "builder": "@nrwl/node:execute",
    "options": {
        "buildTarget": "infra:build",
        "watch": false
    }
},

Expected Behavior

The node application to run once and exit.

Related Issue(s)

Fixes #4054

@nx-cloud
Copy link

nx-cloud bot commented Nov 13, 2020

Nx Cloud Report

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

Status Command Start Time
#000000 nx build-base angular 1/8/2021, 4:39:30 PM
#000000 nx build-base create-nx-plugin 1/8/2021, 4:39:36 PM
#000000 nx build-base create-nx-workspace 1/8/2021, 4:39:23 PM
#000000 nx build-base cypress 1/8/2021, 4:39:16 PM
#000000 nx build-base dep-graph-client --configuration release 1/8/2021, 4:38:07 PM
#000000 nx build-base eslint-plugin-nx 1/8/2021, 4:39:23 PM
#000000 nx build-base jest 1/8/2021, 4:39:15 PM
#000000 nx build-base linter 1/8/2021, 4:39:15 PM
#000000 nx build-base next 1/8/2021, 4:38:01 PM
#000000 nx build-base nx-plugin 1/8/2021, 4:39:22 PM
#000000 nx build-base react 1/8/2021, 4:39:50 PM
#000000 nx build-base storybook 1/8/2021, 4:39:30 PM
#000000 nx build-base web 1/8/2021, 4:39:30 PM
#000000 nx build-base workspace 1/8/2021, 4:37:58 PM
#000000 nx run-many --target=build --all --parallel 1/8/2021, 4:36:17 PM
#000000 nx run-many --target=e2e --projects=e2e-node 1/8/2021, 4:38:13 PM
#000000 nx run-many --target=lint --all --parallel 1/8/2021, 4:40:29 PM
#000000 nx run-many --target=test --all --parallel 1/8/2021, 4:36:08 PM

Sent with 💌 from NxCloud.

Copy link
Member

@Cammisuli Cammisuli left a comment

Choose a reason for hiding this comment

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

Hmm.. we'll probably need a migration for this. Existing projects should have their workspace.json updated so that watch: true is set, and doesn't break any existing expectations.

New projects will not have watch by default.

If you need help with the migration, let me know and I can guide you!

@CurtisHughes
Copy link
Contributor Author

Sounds good, I'll take a look at adding the migration and reach out if I need any help. Thanks @Cammisuli

@CurtisHughes
Copy link
Contributor Author

@Cammisuli I took at shot at the migration (mainly following set-build-libs-from-source.ts as a reference).

Wasn't sure what version this would fall under so I just used 10.4.5 assuming it would be in the next patch. Please let me know if I should change that to something else.

@Cammisuli
Copy link
Member

Alright, thank you! We'll probably get this in after v11. I'll check up again soon.

@Cammisuli
Copy link
Member

I'm finally coming around to this and I seem to have found an issue with the execute watch setting not actually working...

I'll see if I can fix it with this pr.

Thanks again for your patience!

@Cammisuli
Copy link
Member

So I had to remove the migration because the underlying issue is actually fixed. Now we pass the proper option from the execute builder to whatever underlying builder that's being called.

I also changed a couple more things so that things are properly watched and reloaded when you do want to have watch: true

@CurtisHughes
Copy link
Contributor Author

Nice! Thanks for pushing those changes @Cammisuli
Let me know if there is anything else that needs to be done and I can work on.

@Cammisuli Cammisuli changed the title fix(node): remove watch option hardcoded value fix(node): use proper watch value with the node:execute executor Jan 8, 2021
@Cammisuli Cammisuli merged commit 21bd952 into nrwl:master Jan 8, 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.

Node script not exiting
2 participants