search

riot / cli

Riot.js command line tool
MIT License
46 stars 9 forks source link

Debounce compilation on watch (100ms). #29

Closed millette closed 6 years ago

millette commented 6 years ago

Currently, if you're watching 5 tags and save 2 simultaneously in your editor, the 5 tags are compiled twice (once for each file that changed). With a short debounce, we get a single compilation pass.

millette commented 6 years ago

For some reason lodash.debounce isn't getting installed (not found in node_modules/) so I tried again with the full lodash.

Running the tests locally, I get this error:

stdin -> test/generated/stdinout/output.js
make: ./node_modules/.bin/istanbul: commande introuvable
Makefile:40: recipe for target 'test-mocha' failed
make: *** [test-mocha] Error 127
error Command failed with exit code 2.
coveralls commented 6 years ago

Coverage Status

Coverage remained the same at 83.871% when pulling edb8fe2a2a61f12d1b067fc76693f3b2521c6f32 on millette:debounce into d72182cff9001d43ce4b683293e901f6ce3c90fd on riot:master.

millette commented 6 years ago

Finally!

I'd appreciate it if someone can tell me why require('lodash).debounce works, but require('lodash.debounce) doesn't.

GianlucaGuarini commented 6 years ago

hi @millette thanks for your pull request but I would like to avoid lodash dependencies, please use this simple debounce implementation

millette commented 6 years ago

Should I simply copy that implementation into tasks/watch.js ?

millette commented 6 years ago

Note also that lodash is already a dependency down the chain.

$ yarn why lodash
...
[4/4] Calculating file sizes...
info Reasons this module exists
   - "riot-compiler#string-similarity" depends on it
   - "eslint" depends on it
   - "eslint#inquirer" depends on it
millette commented 6 years ago

I don't know how picky you are about PRs. Should I include https://github.com/millette/cli/pull/1/commits/c8c2c28dc0c0faeee595bc4fece9dc7b41f1ff88 in the current PR or provide a second one?

GianlucaGuarini commented 6 years ago

@millette please add the simple debounce method somewhere here and i will be happy to merge your PR

millette commented 6 years ago

Regarding https://github.com/riot/cli/pull/29#issuecomment-353806160 do you want a 2nd PR to replace babel with babel-cli and coffee-script with coffeescript?

coveralls commented 6 years ago

Coverage Status

Coverage increased (+0.3%) to 84.127% when pulling fad4e0f6b3fdbd4265f7219e918bee6d7fc7430f on millette:debounce into d72182cff9001d43ce4b683293e901f6ce3c90fd on riot:master.

GianlucaGuarini commented 6 years ago

thank you I need to work on the repo anyway so your PR is enough for now