fix: live reload port fallback if port is used #899
Conversation
|
Deploy preview for docusaurus-preview ready! Built with commit febf0d5 |
There was a problem hiding this comment.
@tom-auger I love that you did this.
In general, this looks overall better to me. I want a 2nd pair of eyes for confirmation.
lib/__tests__/findUnusedPort.test.js
Outdated
| @@ -0,0 +1,83 @@ | |||
| /** | |||
There was a problem hiding this comment.
❤️ that you are adding tests.
| module.exports = { | ||
| LIVE_RELOAD_PORT: 35729, | ||
| const tinylrServer = { | ||
| listen: jest.fn(), |
There was a problem hiding this comment.
This file has no relationship to the constants file, right? You just changed the constants file and then made a file move. I am just wondering why GitHub thinks this is a file move.
There was a problem hiding this comment.
That's right, the two files are unrelated. I actually deleted the constants file in a separate commit, and added the tiny-lr mock in a later one, so I'm not sure why GitHub thinks this was a file move 😕
There was a problem hiding this comment.
Skimmed through it, seems ok for me.
Something to consider:
- Use https://www.npmjs.com/package/portfinder instead of building our own. But this PR's
findUnusedPortlooks OK to me - I think
lib/serverController.jsshould be namedlib/server/startor at least moved tolib/serverfolder since ideally we want all thedocusaurus-<command>to be onlibfolder and not others
endiliey
changed the title
There was a problem hiding this comment.
@tom-auger This is awesome! I think the tests are quite overkill since this is a non-essential feature and pretty simple to test. But I absolutely love that you're adding them. Do you have time to respond to the comments by @endiliey? If not I think we could fix it for you and merge it. Let us know 😄
|
@yangshun thanks 😃, sure I can fix it up! RE the comments by @endiliey:
|
|
@yangshun I've made the suggested changes. I fixed the tests too, but if you don't want to keep them feel free to remove them 😄 |
Motivation
Fixes #731
Have you read the Contributing Guidelines on pull requests?
Yes.
Test Plan
Related PRs
N/A