Skip to content

change exports to a factory#7

Merged
daquinoaldo merged 2 commits into
daquinoaldo:masterfrom
stutrek:master
Mar 25, 2019
Merged

change exports to a factory#7
daquinoaldo merged 2 commits into
daquinoaldo:masterfrom
stutrek:master

Conversation

@stutrek

@stutrek stutrek commented Mar 24, 2019

Copy link
Copy Markdown
Contributor

PR Details

This makes the exports of the module a factory function rather than exporting app directly. Doing this brings the API in line with express and allows you to create more than one app in a node process.

This is a breaking change, but could be converted to a non-breaking change by continuing to export the app and adding the factory function to it. I decided against that because I thought people would expect it to work like express.

Motivation and Context

I maintain a development server that allows you to start multiple http servers with webpack-dev-middleware using a single command. The use cases are:

  • If you're creating two versions of an app (maybe a lite and a pro, or for two different user types) this allows you to run both.
  • You can run things like storybook or other testing environments.
  • You can run multiple back end process simultaneously.

How Has This Been Tested

Unit tests pass. I am using it locally in my dev server.

Types of changes

  • Docs change / refactoring / dependency upgrade
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document. (does not exist)
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@coveralls

coveralls commented Mar 24, 2019

Copy link
Copy Markdown

Coverage Status

Coverage remained the same at 100.0% when pulling 9e3189a on stutrek:master into b50d108 on daquinoaldo:master.

@daquinoaldo

Copy link
Copy Markdown
Owner

Thank you for your work!
I'll review your PR as soon as I can!

@daquinoaldo daquinoaldo self-requested a review March 25, 2019 15:44
@daquinoaldo daquinoaldo self-assigned this Mar 25, 2019
@daquinoaldo daquinoaldo added the feature New feature or request label Mar 25, 2019
@daquinoaldo daquinoaldo merged commit 6fecbb5 into daquinoaldo:master Mar 25, 2019
@daquinoaldo daquinoaldo removed their request for review March 25, 2019 16:02
@daquinoaldo

Copy link
Copy Markdown
Owner

Merged!
Thank you for your contribution and for all the details that you provide.

@daquinoaldo

Copy link
Copy Markdown
Owner

Published on npm with tag v4.0.0.

@stutrek

stutrek commented Mar 25, 2019

Copy link
Copy Markdown
Contributor Author

Thanks!

I was thinking about this last night, I will want it to ask for a password (if required) when you first run the server, rather than when it's installed. If you're open I'll try to make that change next weekend.

@daquinoaldo

Copy link
Copy Markdown
Owner

The password is the sudo password I suppose.
It is needed to generate the certificates, so you are proposing to generate certificates at first run?

@stutrek

stutrek commented Mar 25, 2019

Copy link
Copy Markdown
Contributor Author

Yes, exactly. With a message "Please enter your password to enable https" or something similar.

@daquinoaldo

Copy link
Copy Markdown
Owner

Make sense. Can you implement that?

I also open an issue #8 that I would like to implement sooner or later. If it you would like to take a look, I think it could be easier to implement with your proposal edits, otherwise I'll implement myself when I have time.

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

Labels

feature New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants