-
Notifications
You must be signed in to change notification settings - Fork 393
fix: move config watcher to graph command #4362
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
Conversation
@@ -486,8 +511,10 @@ const createDevCommand = (program) => { | |||
'netlify dev', | |||
'netlify dev -d public', | |||
'netlify dev -c "hugo server -w" --targetPort 1313', | |||
'netlify dev --graph', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
drive-by adding a new example
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You'll need to run npm run docs
to sync the docs with this change (and fix the CI)
📊 Benchmark resultsComparing with 3db51e2 Package size: 443 MB⬇️ 0.00% decrease vs. 3db51e2
Legend
|
const configWatcher = new events.EventEmitter() | ||
|
||
// Only set up a watcher if we know the config path. | ||
if (configPath) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think about keeping this in the base command, and only applying for dev
?
I'm thinking we might want this for functions:serve
and possibly other long running commands.
I'll leave that up to you, I don't think it's a blocker for the PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd prefer to do that work when we need that for functions:serve
. There are (as evidenced by this PR) wrinkles to figure out wrt memory usage and I believe exposing this in BaseCommand
could give false confidence that this is ready for wide application.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm - thanks for helping out our users!
🎉 Thanks for submitting a pull request! 🎉
Summary
Tentative fix for #4359
For us to review and ship your PR efficiently, please perform the following steps:
passes our tests.
A picture of a cute animal (not mandatory, but encouraged)