Conversation
|
Hi @emilbayes I can't review and test today, but will give it a try tomorrow. Thanks for your PR, from a first view, it looks very good. |
|
No worries. The PR includes test too btw |
|
I quickly reviewed your PR. I'll probably merge later today, thanks for including all these tests as well. |
mklabs
left a comment
mklabs
left a comment
There was a problem hiding this comment.
Overal, great PR. I requested some minor changes if you don't mind changing a bit your code, mainly to de-duplicate the tests.
| import listen from './helpers/listen'; | ||
| import {PassThrough} from 'stream'; | ||
|
|
||
| describe('tiny-lr', () => { |
There was a problem hiding this comment.
Can you change the description to something like tiny-lr custom livereload to differentiate it from the other tests in the test output ?
| return s; | ||
| } | ||
| })); | ||
|
|
There was a problem hiding this comment.
All the below tests, except maybe one GET /livereload.js, are copy-pasted from server.js. Can you try to refactor these two files to use a function passed to the parent describe in order to not duplicate these tests ?
Something like:
describe('tiny-lr custom livereload', () => {
before(...)
testLR(this);
// divergent tests here, mainly GET /livereload.js
});with testLR including all common tests from these two files. and using a param like context to map the this passed as a parameter.
lib/server.js
Outdated
| this.options = options; | ||
|
|
||
| options.livereload = options.livereload || require.resolve('livereload-js/dist/livereload.js'); | ||
| options.livereload = typeof options.livereload === 'string' |
There was a problem hiding this comment.
Can you put ? ... at the end of the line instead of line breaking before ? ?
options.livereload = typeof options.livereload === 'string' ? wrapStream(options.livereload)
: typeof options.livereload === 'function' ? options.livereload
: wrapStream(require.resolve('livereload-js/dist/livereload.js'));
|
Sorry for the late reply. The requested changes have been made 😄 |
|
@mklabs Ping :) |
|
@mklabs Any further changes I need to do? |
|
@emilbayes it looks like you have some merge conflicts to resolve, but I'm happy to take care of merging this into our upcoming release if you can update the PR! Sorry for the extremely slow response. <3 |
3 participants
I realise this might not be something you're after, but maybe we can find common ground and have a feature that leaves more flexibility.
I want to bundle livereload on the fly with browserify as I've got some extra plugins that go with the stock version. Allowing one to pass a function that returns a stream was the least invasive solution I could come up with, that was also easy to code. I don't know if you have plans for a similar feature, but implemented differently? I'd be open to do the work, if we can find a solution that everyone is happy with.
This PR should be semver minor, as it preserves the existing API but also checks for
opts.livereloadbeing a function.