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 cjs modules by default #1957

Closed
wants to merge 1 commit into from

Conversation

lorand-horvath
Copy link

@remy
Copy link
Owner

remy commented Sep 16, 2022

I'm worried this change looks simple but possibly hiding impact, such as on people's defaults or on what happens ordinarily without it.

I don't have the time ATM to give it a good test, but it's obviously easy to add this to your own projects to work around it.

@remy
Copy link
Owner

remy commented Jul 7, 2023

I've moved this in to the latest release. It should be nodemon@3.

@remy remy closed this Jul 7, 2023
@lorand-horvath
Copy link
Author

lorand-horvath commented Jul 7, 2023

@remy Sorry for having to say this upfront, but I was very much expecting for you to actually merge my PR, which was waiting for you to make up your mind and include *.cjs files as default watch since November 2022. For some reason you refused initially, but now all of a sudden it's fine to use it. Instead of merging and linking my PR, all you did was thank me with a general @ in your commit 86d5f40 which will be untraceable within a few days already.

I know it's not a big deal, but that's not how open source acknowledgements are normally carried out and I think it discourages people from collaborating or supporting each other.

I will completely uninstall nodemon from my dev machines at this point and just use node 18 --watch mode. I strongly urge others to do the same.

If you think I have made a mistake and perhaps am missing something here, please let me know.

@remy
Copy link
Owner

remy commented Jul 8, 2023

It wasn't really all of a sudden! But it was because it was barely a change. The actual work, that you didn't do in the pr, which is why I was wary, was the testing.

This is what I did on Friday, I created a series of simple test cases and ran them against the change.

It's always the tests that show the work.

But, please do uninstall nodemon. Using node 18's native watch gets you pretty far, I'm sure it'll serve you well (and do share the word about nodes native reload, it helps others so that's not a bad thing).

Hopefully that helps you understand where the real work was, and why I wasn't sure whether it was a safe pr to merge (without having to do the leg work myself back in Novem).

remy added a commit that referenced this pull request Jul 9, 2023
@remy
Copy link
Owner

remy commented Jul 9, 2023

For what it's worth, I didn't manage to fully test this (because I don't use typescript at all), someone caught that it broke default watching for TS: #2124

So I baked the cjs watching directly into the code, seen here: 95bee00#diff-d649d3b2f2afcc1dfc4d0dce7c28b4fc296860b45dd38d3c7bb0274a6be73708L117 (and then of course tested again)

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

Successfully merging this pull request may close these issues.

2 participants