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

change exports to a factory #7

Merged
merged 2 commits into from
Mar 25, 2019
Merged

Conversation

stutrek
Copy link
Contributor

@stutrek stutrek commented Mar 24, 2019

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
Copy link

coveralls commented Mar 24, 2019

Coverage Status

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

@daquinoaldo
Copy link
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
Owner

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

@daquinoaldo
Copy link
Owner

Published on npm with tag v4.0.0.

@stutrek
Copy link
Contributor Author

stutrek commented Mar 25, 2019

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
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
Copy link
Contributor Author

stutrek commented Mar 25, 2019

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

@daquinoaldo
Copy link
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.

None yet

3 participants