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: runOnChangeOnly=true #1751

Merged
merged 3 commits into from Oct 4, 2020
Merged

fix: runOnChangeOnly=true #1751

merged 3 commits into from Oct 4, 2020

Conversation

@MonarchChakri
Copy link
Contributor

@MonarchChakri MonarchChakri commented Aug 7, 2020

Updated

  1. test/monitor/run.test.js: to run the test which was previously skipped.
  2. lib/monitor/run.js: to handle the start on change only case (-C/--on-change-only) by adding an early exit.
  3. lib/monitor/watch.js: to precheck config.options.execOptions before accessing script.
Fixed the problem in lib/monitor/run and made this test to run.
Script will create a tmp file, which should not happen as  runOnChangeOnly is set to true.
Moved run.kill outside the run function
Moved up restart bind, before runCmd so that we can use it in run.kill

If command is not to be run the we just need to watch files and not fork/spawn a child process

Binding options with instance of run, so that we can use it in run.kill (https://travis-ci.org/github/remy/nodemon/builds/714612398)
Updated run options inside if condition (https://travis-ci.org/github/remy/nodemon/builds/714620936)
This ensures that config.options.execOptions exists before accessing script (https://travis-ci.org/github/remy/nodemon/builds/714622542)
@stale
Copy link

@stale stale bot commented Aug 22, 2020

This issue has been automatically marked as idle and stale because it hasn't had any recent activity. It will be automtically closed if no further activity occurs. If you think this is wrong, or the problem still persists, just pop a reply in the comments and @remy will (try!) to follow up.
Thank you for contributing <3

@stale stale bot added the stale label Aug 22, 2020
@MonarchChakri
Copy link
Contributor Author

@MonarchChakri MonarchChakri commented Aug 29, 2020

@remy Did you get time to look at this PR?

@stale stale bot removed the stale label Aug 29, 2020
@stale
Copy link

@stale stale bot commented Sep 12, 2020

This issue has been automatically marked as idle and stale because it hasn't had any recent activity. It will be automtically closed if no further activity occurs. If you think this is wrong, or the problem still persists, just pop a reply in the comments and @remy will (try!) to follow up.
Thank you for contributing <3

@stale stale bot added the stale label Sep 12, 2020
@stale stale bot closed this Sep 20, 2020
@remy
Copy link
Owner

@remy remy commented Oct 4, 2020

Caught me at a personal time that I was actively avoiding the computer. I must have looked at it once, but I couldn't see tests for such significant changes. Happy to reopen if there's checks for the specific changes (that either shows fixes have been applied or otherwise).

@MonarchChakri
Copy link
Contributor Author

@MonarchChakri MonarchChakri commented Oct 4, 2020

@remy
This commit was given to fix the issue #1742: --on-change-only / -C options were not working and it was marked as a bug.
Found the root cause of the issue and fixed it in this commit.

Updated the test case to be run now in test/monitor/run.test.js to run the test which was previously skipped.
Also it was not caught by the tests because the test was never actually executed under test/monitor/run.test.js.
Please review and merge.

image

@MonarchChakri
Copy link
Contributor Author

@MonarchChakri MonarchChakri commented Oct 4, 2020

Please verify the test case for runOnChangeOnly under test/monitor/run.test.js

@remy remy reopened this Oct 4, 2020
@stale stale bot removed the stale label Oct 4, 2020
@remy
Copy link
Owner

@remy remy commented Oct 4, 2020

Okay, I'm always super wary of large code changes, even if they're rearranging, but this change still has all tests passing so either the 100s of tests I've written are garbage or your code hasn't broken anything 👍

Merging now.

@remy remy changed the title fix for issue #1742: corrected run.js, run.test.js, watch.js for the use case runOnChangeOnly=true fix: runOnChangeOnly=true Oct 4, 2020
@remy remy merged commit 7e00a30 into remy:master Oct 4, 2020
8 checks passed
8 checks passed
@netlify
Header rules No header rules processed
Details
@netlify
Pages changed All files already uploaded
Details
@netlify
Redirect rules No redirect rules processed
Details
@netlify
Mixed content No mixed content detected
Details
@remy
ci/semantic-release expected next release: 2.0.5
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@netlify
deploy/netlify Deploy preview ready!
Details
security/snyk (remy) No manifest changes detected in 1 project
Details
@MonarchChakri
Copy link
Contributor Author

@MonarchChakri MonarchChakri commented Oct 4, 2020

Thanks for merging and adding the label hacktoberfest-accepted. 😄
But as per the rules I need to submit a PR during October and I have submitted it long back.
Maybe assign me another issue which I can work on it or I'll try next time. 😅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants