Skip to content

Conversation

@prantlf
Copy link

@prantlf prantlf commented May 28, 2018

Attempts to fix #1.

Copy link
Owner

@bennieswart bennieswart left a comment

Choose a reason for hiding this comment

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

@prantlf thanks for this PR!

I can see how the change you're proposing can be useful when one might require different versions or different configurations of vnu.

Would you mind splitting this PR into two parts - one for the functional change which adds the new parameter, and another for the build process changes, stylistic rules etc.

A general comment about the proposed change: I see that grunt-html does not (yet) support a function for the server option. Compatibility with grunt-html off-the-shelf is important, so I'd rather not break that. I do think, however, that changing the options of grunt-html after the port has been determined should be possible. See if you can do it that way.
Additionally, it is probably sensible to expose the MAX_PORTS setting in some way. How about overloading the useAvailablePort setting such that true and { max: 30 } are both valid?

@prantlf prantlf force-pushed the use-available-port branch from 9ce256b to 603dd5d Compare May 29, 2018 05:14
…is in use

This helps, if multiple projects are built on the same machine, which use
the vnu server internally to speed up the HTML file validation.
@prantlf prantlf force-pushed the use-available-port branch from 603dd5d to 4ccf50e Compare May 29, 2018 05:21
@prantlf
Copy link
Author

prantlf commented May 29, 2018

Thanks for reviewing my change so quickly!

OK, I removed the checks and test. I can send it later.

I do intend to send a PR to grunt-html to support computing the server parameters dynamically by a function. It is the dynamic-server-option branch, I don't see it as the chicken-or-egg problem, but rather grunt-vnuserver being the egg, which needs to come first and grunt-html, which should let it hatch ;-)

Yes, I did initially introduce a parameter to configure the MAX_PORT value. (I added maxPort to the task options, but it is not so important.) Later I decided to remove it, not to put a new configuration on the public interface, which wouldn't be useful. Giving it a second thought, I wasn't sure, if anybody would want to increase the maximum value, The grunt-contrib-connect task doesn't make it configurable either. Do you believe, that it is worth it?

@bennieswart
Copy link
Owner

Nice, the PR is much more cohesive now.

With regards to the MAX_PORT value, having it hard-coded will definitely suffice in most situations. It would, however, require another change is someone ever wishes to change it in the config, so we might as well cover that now, since it is such an easy thing to support.

For setting the port dynamicallly, I'm still leaning towards the side where the config is updated dynamically after the port has been determined. This way, no changes are required to grunt-html (as well as any other projects that use grunt-vnuserver. It's a more generic solution and would ensure compatibility with any other project without requiring changes there, so it's definitely the right way to go.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Start vnu server on the first available port

2 participants