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

Allow all configuration options of livereload to be passed through. #246

Merged
merged 4 commits into from Sep 18, 2018

Conversation

@aknrdureegaesr
Copy link
Contributor

@aknrdureegaesr aknrdureegaesr commented Aug 23, 2018

Allow all livereload configuration options to be passed through.

Fixes #100

@jsf-clabot
Copy link

@jsf-clabot jsf-clabot commented Aug 23, 2018

CLA assistant check
All committers have signed the CLA.

CHANGELOG Outdated
@@ -1,3 +1,7 @@
v1.1.0:

This comment has been minimized.

@XhmikosR

XhmikosR Sep 6, 2018
Member

Please leave all version changes to the person who will make the release if this is merged.

@XhmikosR XhmikosR force-pushed the innoq:general_connect_livereload branch from f7d862d to 1b2f13f Sep 9, 2018
@XhmikosR XhmikosR force-pushed the innoq:general_connect_livereload branch 2 times, most recently from 520f07b to d856f0e Sep 9, 2018
@XhmikosR
Copy link
Member

@XhmikosR XhmikosR commented Sep 9, 2018

@aknrdureegaesr Can you reduce duplication a bit? hostname: options.hostname is set in both cases.

Also can you add a test?

@aknrdureegaesr
Copy link
Contributor Author

@aknrdureegaesr aknrdureegaesr commented Sep 9, 2018

I have been looking for the test to augment by temporarily introducing a bug in the existing passing of the parameters:

        // Inject live reload snippet
        if (options.livereload !== false) {
          middleware.unshift(injectLiveReload({port: 35729, hostname: 'localhost'}));
          callback(null);
        } else {
          callback(null);
        }

When I run npm test, all tests still pass.

Is there any other way to run tests, or does a test that tests parameter passing simply not exist presently?

@XhmikosR
Copy link
Member

@XhmikosR XhmikosR commented Sep 9, 2018

The tests are here

livereload: function(test) {
test.expect(2);
get({
hostname: 'localhost',
port: 8006,
path: '/livereload.html',
headers: {
accept: 'text/html'
}
}, function(res, body) {
test.ok(body.indexOf('35729/livereload.js') !== -1, 'Should contain livereload snippet.');
// check if livereload works with params
get({
hostname: 'localhost',
port: 8006,
path: '/livereload.html?a=1&b=2#id',
headers: {
accept: 'text/html'
}
}, function(res, body) {
test.ok(body.indexOf('35729/livereload.js') !== -1, 'Should contain livereload snippet.');
test.done();
});
});
},

You could add a new test there, pass your options and check if the port/hostname etc match.

@aknrdureegaesr
Copy link
Contributor Author

@aknrdureegaesr aknrdureegaesr commented Sep 9, 2018

There are three code branches:

  • options.livereload is true
  • options.livereload is a number
  • options.livereload is a map.

The redundant hostname: options.hostname is only set in two of the three.

@XhmikosR
Copy link
Member

@XhmikosR XhmikosR commented Sep 9, 2018

The redundant hostname: options.hostname is only set in two of the three.

Yeah, but still, it's duplication.

@aknrdureegaesr
Copy link
Contributor Author

@aknrdureegaesr aknrdureegaesr commented Sep 9, 2018

Yes, I've seen those tests. Those tests do not test whether the number passed as a port is actually used, so they test something, but not parameter handover.

I take your answer as "tests that test parameter passing do not presently exist".

I'll see what I can do.

@aknrdureegaesr
Copy link
Contributor Author

@aknrdureegaesr aknrdureegaesr commented Sep 9, 2018

The way to avoid it I can think of is something like

        if (options.livereload !== false) {
          if (options.livereload === true || typeof options.livereload === 'number') {
            ...
            if (options.livereload === true) {
              ...
            } else {
              ...
            }
          }

Seems to me that cure is worse than the disease. What do you think?

@XhmikosR
Copy link
Member

@XhmikosR XhmikosR commented Sep 9, 2018

OK, let's forget about the deduplication, but please try to add a test.

Andreas Krüger
@aknrdureegaesr aknrdureegaesr force-pushed the innoq:general_connect_livereload branch from 50bf15d to 5cdf2d6 Sep 17, 2018
@aknrdureegaesr
Copy link
Contributor Author

@aknrdureegaesr aknrdureegaesr commented Sep 17, 2018

OK, let's forget about the deduplication, but please try to add a test.

Done.


*This does not perform live reloading. It is intended to be used in tandem with grunt-contrib-watch or another task that will trigger a live reload server upon files changing.*
If you set to `true`, defaults are used. If you set to a number, that number is used as a port number, together with the hostname you configured. If you set this to a map, that map is passed to `connect-livereload` unchanged as its configuration.

This comment has been minimized.

@XhmikosR

XhmikosR Sep 17, 2018
Member

map -> object

Andreas Krüger added 2 commits Sep 17, 2018
Andreas Krüger
…h -1.
@aknrdureegaesr aknrdureegaesr force-pushed the innoq:general_connect_livereload branch from 5cdf2d6 to f320a47 Sep 17, 2018
@XhmikosR
Copy link
Member

@XhmikosR XhmikosR commented Sep 17, 2018

So, @aknrdureegaesr, thanks for the patience so far!

Now, I was wondering, the 2 tests you added do seem indentical in the objects you are passing. Do we need them to be like that or should we test for different things?

@aknrdureegaesr
Copy link
Contributor Author

@aknrdureegaesr aknrdureegaesr commented Sep 17, 2018

At this point, we have three cases: Livereload is simply true, or is a number, or is an object. The possibility to add an object is new. The other two configuration options existed before.

As I found out, there was no previous test for the "number" case. There was only a test on the "true" case.

So I added that missing test for the previously-existing "number" functionality. And I also added a test for my own work. So we now have three tests instead of one.
The two new tests very closely model what had been done in the previously existing test.

But...

I am not so sure the previously existing functionality is actually useable. At least on my machine, in a real-world example, both variants (true and number) kept producing a script URI along the lines of http://0.0.0.0:port/.... My browser could not successfully fetch a script from 0.0.0.0. The patch I present here is helpful, as it allows to precisely determine the URI.

It might be worth a consideration to simply remove the old "true" and the old "port" configuration. There is nothing you could do with those that you couldn't do with the full way, that my patch introduces.

But it might break compatibility for people who have been using the old way and for whom that worked. I'm not sure whether such people exist or don't. Those people would have to re-configure.

In short: To reduce code and tests, we could remove the two old ways, "true" and number, and only keep my new way. I'd be happy to reduce the code in that way.

What do you think, is better? Small, or compatible?

@XhmikosR
Copy link
Member

@XhmikosR XhmikosR commented Sep 18, 2018

Let's keep the changes to the minimum here and you can always make a new PR to improve tests later.

…xOf" with -1."

This reverts commit f320a47.
@XhmikosR XhmikosR merged commit d5390ff into gruntjs:master Sep 18, 2018
3 checks passed
3 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
licence/cla Contributor License Agreement is signed.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants