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

FileResourceServer must be explicitly enabled when initializing Router #1344

Merged
merged 5 commits into from Oct 15, 2018

Conversation

Projects
None yet
4 participants
@NocturnalSolutions
Copy link
Contributor

NocturnalSolutions commented Oct 3, 2018

Description

This is a redo of #1343 based on feedback in that thread. Whereas the code in that PR implicitly disabled the FileResourceServer for a Router if that Router had any elements/handlers, this PR changes Router to add a new optional withResourceServer parameter to its initializer which only starts the FileResourceServer if explicitly set to true.

Motivation and Context

@djones6 recommended this approach rather than the one I had chosen in the original PR, and I like his idea better than mine too. As stated there, this may break expected functionality with the Kitura CLI tool.

How Has This Been Tested?

As with the other PR, existing tests testing the FileResourceServer functionality have been tweaked, and a new test has been added to ensure the functionality is not present by default.

Checklist:

  • If applicable, I have updated the documentation accordingly.
  • If applicable, I have added tests to cover my changes.

NocturnalSolutions added some commits Oct 3, 2018

FileResourceServer must now be explicitly enabled when initializing a…
… Router instance in order to serve Kitura resources (the "Welcome to Kitura" page and related files).

@djones6 djones6 self-requested a review Oct 5, 2018

/// FileResourceServer to serve the default
/// "Welcome to Kitura" page and related
/// assets.
public init(mergeParameters: Bool = false, useResourceServer: Bool = false) {

This comment has been minimized.

@djones6

djones6 Oct 5, 2018

Member

Please can we name this param enableWelcomePage? I think it is more descriptive (also the 'ResourceServer' is an internal implementation detail).

Also, having considered it more, the default should be true, to make this an opt-out rather than an opt-in. We want to maintain the existing behaviour by default, otherwise this would have to wait until Kitura 3. (we could consider turning it off at that time).

@@ -34,6 +34,7 @@ class TestStaticFileServer: KituraTest {
("testGetWithWhiteSpaces", testGetWithWhiteSpaces),
("testGetWithSpecialCharacters", testGetWithSpecialCharacters),
("testGetWithSpecialCharactersEncoded", testGetWithSpecialCharactersEncoded),
("testNoResourcesByDefault", testNoResourcesByDefault),

This comment has been minimized.

@djones6

djones6 Oct 5, 2018

Member

as per the previous comment, this would become testWelcomePageCanBeDisabled

Rename the new useResourceServer parameter on Router.init() to enable…
…WelcomePage and have it be true, rather than false, by default
@NocturnalSolutions

This comment has been minimized.

Copy link
Contributor

NocturnalSolutions commented Oct 5, 2018

Okay, tweaks made and pushed. But yes, I'm hoping this latest commit can be reverted for Kitura 3, as as I've said before, the current behavior is arguably a bug.

@djones6

This comment has been minimized.

Copy link
Member

djones6 commented Oct 10, 2018

@NocturnalSolutions this looks good, thanks. I'm pushing a small change to reverse the order in which we check whether there's a FileResourceServer vs. checking the path, since the latter is more expensive and we can skip it if the welcome page is disabled.

djones6 added some commits Oct 10, 2018

@djones6 djones6 merged commit 55c0cf5 into IBM-Swift:master Oct 15, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
license/cla Contributor License Agreement is signed.
Details
@TaborKelly

This comment has been minimized.

Copy link

TaborKelly commented Nov 30, 2018

Is there a timeline on when this will be available for a released version of Kitura?

@ianpartridge

This comment has been minimized.

Copy link
Member

ianpartridge commented Nov 30, 2018

Perhaps next week, or maybe the week after.

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