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

misc: add user-defined chokidar files to watch (2) #3633

Merged
merged 56 commits into from
Aug 8, 2018
Merged

misc: add user-defined chokidar files to watch (2) #3633

merged 56 commits into from
Aug 8, 2018

Conversation

galvez
Copy link

@galvez galvez commented Aug 5, 2018

@Atinux @clarkdo This is me picking up on this PR, which seems abandoned. I've refactored CLI tests a bit and added a nuxt dev test to make sure the custom chokidar watchers were working.

cc @SabatinoMasala

@codecov-io
Copy link

codecov-io commented Aug 5, 2018

Codecov Report

Merging #3633 into dev will decrease coverage by 0.25%.
The diff coverage is 66.66%.

Impacted file tree graph

@@            Coverage Diff            @@
##             dev    #3633      +/-   ##
=========================================
- Coverage   99.2%   98.94%   -0.26%     
=========================================
  Files         18       18              
  Lines       1125     1134       +9     
  Branches     303      304       +1     
=========================================
+ Hits        1116     1122       +6     
- Misses         9       12       +3
Impacted Files Coverage Δ
lib/common/nuxt.config.js 100% <ø> (ø) ⬆️
lib/builder/builder.js 98.42% <66.66%> (-1.58%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7349add...0c2c754. Read the comment docs.

Copy link
Member

@Atinux Atinux left a comment

Choose a reason for hiding this comment

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

Small changes required, but otherwise, amazing work!

@@ -622,6 +623,15 @@ export default class Builder {
this.customFilesWatcher = chokidar
.watch(customPatterns, options)
.on('change', refreshFiles)

// Watch for nuxt.config.js and user-defined changes
const nuxtConfigPatterns = _.concat(
Copy link
Member

Choose a reason for hiding this comment

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

I don't think that nuxtConfigPatterns is the right name, maybe something like nuxtRestartWatch

@@ -633,6 +643,10 @@ export default class Builder {
this.customFilesWatcher.close()
}

if (this.nuxtConfigWatcher) {
this.nuxtConfigWatcher.close()
Copy link
Member

Choose a reason for hiding this comment

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

Same here, nuxtRestartWatcher is more understandable

@@ -195,5 +195,8 @@ Options.from = function (_options) {
options.build.transpile = [].concat(options.build.transpile || [])
.map(module => module instanceof RegExp ? module : new RegExp(module))

// User-defined changes
options.watch = []
Copy link
Member

Choose a reason for hiding this comment

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

I think this should move to https://github.com/nuxt/nuxt.js/blob/dev/lib/common/nuxt.config.js (I will use this file in the docs to display all configurations)

@@ -633,6 +643,10 @@ export default class Builder {
this.customFilesWatcher.close()
}

if (this.nuxtConfigWatcher) {
Copy link
Member

Choose a reason for hiding this comment

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

Can't we reuse customPatterns, is there any special reason for a separated watcher ?

)
this.nuxtConfigWatcher = chokidar
.watch(nuxtConfigPatterns, options)
.on('change', refreshFiles)
Copy link
Member

Choose a reason for hiding this comment

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

If the purpose is to restart the server, can refreshFiles work here ?

Copy link
Member

Choose a reason for hiding this comment

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

Woops, good watch, no we need to sent an event from nuxt internal that nuxt-dev will listen to kill Nuxt and restart it

bin/nuxt-dev Outdated
@@ -128,6 +110,15 @@ function startDev(oldInstance) {
if (oldInstance) {
return nuxt.listen(port, host)
} else {
nuxt.hook('watcher:restart', () => {
Copy link
Member

@clarkdo clarkdo Aug 8, 2018

Choose a reason for hiding this comment

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

If we move the hook out of startDev and under the startDev(), will it work ?

And also watcher:restart should have a more proper name which describe the hook scenario (not restart, restart is just one hook purpose, this hook can be used for any purpose)

@clarkdo clarkdo merged commit a522aaf into nuxt:dev Aug 8, 2018
clarkdo pushed a commit that referenced this pull request Aug 10, 2018
)
this.nuxtRestartWatcher = chokidar
.watch(nuxtRestartWatch, options)
.on('change', (event, _path) => {

Choose a reason for hiding this comment

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

@galvez npm run dev is crashing with the following error every time nuxt.config.js is modified:

path.js:39
    throw new ERR_INVALID_ARG_TYPE('path', 'string', path);
    ^

TypeError [ERR_INVALID_ARG_TYPE]: The "path" argument must be of type string. Received type object
    at assertPath (path.js:39:11)
    at Object.parse (path.js:910:5)
    at FSWatcher.watchers.restart.chokidar.watch.on (E:\Repositories\website\node_modules\nuxt-edge\dist\nuxt.js:3868:42)
    at FSWatcher.emit (events.js:182:13)
    at FSWatcher.EventEmitter.emit (domain.js:442:20)
    at FSWatcher.<anonymous> (E:\Repositories\website\node_modules\chokidar\index.js:199:15)
    at FSWatcher._emit (E:\Repositories\website\node_modules\chokidar\index.js:241:5)
    at FSWatcher.<anonymous> (E:\Repositories\website\node_modules\chokidar\lib\nodefs-handler.js:267:16)
    at FSReqWrap.oncomplete (fs.js:159:5)

According the chokidar docs, the first argument should be the path and the second one the stats:

// 'add', 'addDir' and 'change' events also receive stat() results as second
// argument when available: http://nodejs.org/api/fs.html#fs_class_fs_stats
watcher.on('change', (path, stats) => {
  if (stats) console.log(`File ${path} changed size to ${stats.size}`);
});

Copy link
Author

Choose a reason for hiding this comment

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

@simonbrunel thanks for reporting! will fix ASAP :)

Copy link
Author

Choose a reason for hiding this comment

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

@simonbrunel we just merged a fix for this, thanks again.

Choose a reason for hiding this comment

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

Thanks :)

clarkdo pushed a commit that referenced this pull request Aug 11, 2018
@clarkdo need to merge this asap -- related to #3633 (comment)
@lock
Copy link

lock bot commented Oct 31, 2018

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators Oct 31, 2018
@danielroe danielroe added the 2.x label Jan 18, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants