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

Adding dev server overrides #32

Merged
merged 7 commits into from
Sep 21, 2020

Conversation

cereallarceny
Copy link
Contributor

I added the ability to add overrides to the default dev server configuration. This should only be merged after #31

@cereallarceny
Copy link
Contributor Author

@yusinto I've updated this PR to have all the changes from #31 and now is much smaller. This simply adds the ability to all the dev server to be overridden. I've already confirmed this works on my own local setup and have written tests for this. I believe it's very safe to merge and you can continue closing all other PR's after merging this one.

@cereallarceny
Copy link
Contributor Author

@yusinto I have also now repaired the build from linting discrepancies. They should now be fixed in this PR.

Copy link
Owner

@yusinto yusinto left a comment

Choose a reason for hiding this comment

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

Hi @cereallarceny thanks for this, I've left a comment here so please take a look. Thank you.

@cereallarceny
Copy link
Contributor Author

@yusinto I'm happy for us to use your logger. It doesn't matter to me what logging class we use - but I figured mine would be good enough for most use-cases. I just want to turn off your logs. If you want to add a finer level of control, then you can do so in a future PR. For now though, I think this offers reasonable functionality.

@yusinto
Copy link
Owner

yusinto commented Sep 14, 2020

If you want to add a finer level of control, then you can do so in a future PR.

I'm happy to do the custom logger bit in the future. In which case for this PR, perhaps you can omit the verbose options for now until I have that in place. This does mean you'll have to put up with the console logs until my pr comes around though, so apologies if advance for that if this bothers you tremendously.

@cereallarceny
Copy link
Contributor Author

@yusinto Yes, that does disappoint me. Silencing all the logs of your library's output is the final thing I need to be able to use this library in production. The lack of this feature is preventing me from launching my own library. I hope you can address this in your own PR soon - otherwise, I'm going to fork this project and make my own changes. I'm just trying to help here. 😄

Either way, I've changed your devServer config to be passed the way you want - you're right, much cleaner! I've also removed the verbose flag so that you may do your PR.

Also, your test suite wasn't passing. I wasn't sure quite why so I fixed a bunch of unrelated problems with the test suite and everything is correct now. Looks like some typos. You can merge now, I've done everything you requested.

@cereallarceny
Copy link
Contributor Author

@yusinto Can we please get this merged? I'm going to need to fork and separately release my own version of this library if we can't hurry along merging pull requests.

@yusinto yusinto merged commit ee034e7 into yusinto:master Sep 21, 2020
@yusinto
Copy link
Owner

yusinto commented Sep 21, 2020

@cereallarceny thanks for your work! I'll create a new release today. Thanks again for helping out.

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.

2 participants