Skip to content
This repository has been archived by the owner on Oct 13, 2022. It is now read-only.

Remove chokidar devDependency #133

Merged
merged 1 commit into from
May 6, 2019

Conversation

cristianl
Copy link
Contributor

From v0.17.0 onwards, Sapper uses cheap-watch instead of chokidar.

Live reloading works when you create a project with npx degit sveltejs/sapper-template#rollup and remove chokidar from the resulting package.json before installing. Nothing else in the project depends on chokidar.

Closes #129

From v0.17.0 onwards, Sapper uses cheap-watch instead of chokidar
@Conduitry
Copy link
Member

I did peek briefly at this the other day, and my main concern was that perhaps rollup would use a peer dep of chokidar for something if it found one. Even with cheap-watch being used for Sapper's own internal watches when creating route manifests, rollup's watch is still used for the main build.

When I tried removing chokidar, things seemed fine, but it was possible I made rollup's watching more flaky in some way by doing this. I'm not clear on what rollup's watch mode does when it doesn't find chokidar, or whether it using chokidar is an opt-in or opt-out thing.

@cristianl
Copy link
Contributor Author

Thanks for looking into it. Seems to be opt-in.

They've run into problems and considered making chokidar the default watcher (e.g. this issue has a repro, but as an app-specific false positive, seems acceptable?)

I'm not knowledgeable of the issues chokidar prevents (what's the impact of its native fsevents module, and which optimizations still apply to the versions of Node that Sapper targets).

@cristianl
Copy link
Contributor Author

I had misread the false default from in the docs. After some unrelated debugging, I can confirm that it's opt-out. It does respect watch.chokidar = false when chokidar is installed, but with the config in this template, it uses chokidar by default.

Close for now?

It'd probably be safer to ask people who use this template on Discord and see if they run into problems when they remove chokidar. (Bit of a bother for a minor thing, but for somebody feeling adventurous and who's run out of other things to do...)

@Conduitry
Copy link
Member

From my tests the other day, it looks like Chokidar is opt-in, even if it's available locally. This agrees with @lukeed's experience with Rollup and Chokidar, and seems to match the docs. I'm going to go ahead an merge this - I really don't think that Chokidar gets used unless you specifically enable it, which is not happening in sapper or sapper-template.

@Conduitry Conduitry merged commit 5d6cbd8 into sveltejs:master May 6, 2019
@cristianl
Copy link
Contributor Author

Thanks.

Could be environment specific. I didn't run into any issues without it, and to be honest, the fsevents module probably introduces more installation/upgrade headaches than going without chokidar.

Here's the call stack on a clean project with the previous revision (before this PR was merged):

handle_result (sapper-t2/node_modules/sapper/dist/chunk-b0d6f7d4.js:230)
(anonymous function) (sapper-t2/node_modules/sapper/dist/chunk-b0d6f7d4.js:417)
(anonymous function) (sapper-t2/node_modules/sapper/dist/core.js:810)
emit (events.js:196)
Watcher.emit (sapper-t2/node_modules/rollup/dist/rollup.js:38619)
(anonymous function) (sapper-t2/node_modules/rollup/dist/rollup.js:38766)
[ Promise.then ]
Task.run (sapper-t2/node_modules/rollup/dist/rollup.js:38765)
(anonymous function) (sapper-t2/node_modules/rollup/dist/rollup.js:38648)
processTicksAndRejections (task_queues.js:89)
[ Promise.then ]
_loop_1 (sapper-t2/node_modules/rollup/dist/rollup.js:38648)
Watcher.run (sapper-t2/node_modules/rollup/dist/rollup.js:38652)
(anonymous function) (sapper-t2/node_modules/rollup/dist/rollup.js:38637)
listOnTimeout (timers.js:531)
processTimers (timers.js:475)
[ Timeout ]
init (inspector_async_hook.js:21)
emitInitNative (async_hooks.js:133)
emitInitScript (async_hooks.js:328)
initAsyncResource (timers.js:147)
Timeout (timers.js:178)
setTimeout (timers.js:142)
Watcher.invalidate (sapper-t2/node_modules/rollup/dist/rollup.js:38632)
Task.invalidate (sapper-t2/node_modules/rollup/dist/rollup.js:38725)
(anonymous function) (sapper-t2/node_modules/rollup/dist/rollup.js:38578)
FileWatcher.trigger (sapper-t2/node_modules/rollup/dist/rollup.js:38577)
handleWatchEvent (sapper-t2/node_modules/rollup/dist/rollup.js:38547)
emit (events.js:196)
(anonymous function) (sapper-t2/node_modules/chokidar/index.js:200)
FSWatcher._emit (sapper-t2/node_modules/chokidar/index.js:241)
(anonymous function) (sapper-t2/node_modules/chokidar/lib/fsevents-handler.js:206)
addOrChange (sapper-t2/node_modules/chokidar/lib/fsevents-handler.js:212)
(anonymous function) (sapper-t2/node_modules/chokidar/lib/fsevents-handler.js:244)
filteredListener (sapper-t2/node_modules/chokidar/lib/fsevents-handler.js:60)
(anonymous function) (sapper-t2/node_modules/chokidar/lib/fsevents-handler.js:85)
(anonymous function) (sapper-t2/node_modules/chokidar/lib/fsevents-handler.js:84)
emit (events.js:196)
(anonymous function) (sapper-t2/node_modules/fsevents/fsevents.js:47)
processImmediate (timers.js:439)
[ Immediate ]
init (inspector_async_hook.js:21)
emitInitNative (async_hooks.js:133)
emitInitScript (async_hooks.js:328)
initAsyncResource (timers.js:147)
Immediate (timers.js:218)
setImmediate (timers.js:274)
handler (sapper-t2/node_modules/fsevents/fsevents.js:46)

@cristianl cristianl deleted the pr-remove-chokidar branch May 6, 2019 18:07
@SwaroopH
Copy link

SwaroopH commented May 15, 2019

@cristianl This is causing random reloads on OSX 10.14.4 using @atom editor. Re-adding chokidar as dev dependency seems to fix it. May be this is one of the edge cases @Conduitry was talking about.

@arxpoetica
Copy link
Member

This is probably extraneous, but in case my problem crops up again, I wanted to make sure this item was cross linked with sveltejs/sapper#703

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.

Chokidar Dev Dependency
4 participants