-
Notifications
You must be signed in to change notification settings - Fork 47
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
Time for Version 2 #62
Conversation
I am unsure how to proceed with travis-cli failing checks. I am completely open to collaborate on this by any means possible. |
Passing travis now. Little painful. Lost NodeJs 0.10 support. Added documentation |
package.json
Outdated
@@ -26,7 +26,6 @@ | |||
"ws": "~1.1.1" | |||
}, | |||
"devDependencies": { | |||
"standard": "^7.1.2" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My reason would be sorta in the form of a rude-isc question: I'm not sure why standard was being used in the first place. Rhetorical, I mean I do know why. I find it constrictive and unnecessary. I think my entire pull request is asking a lot of the reload team. But like this major change, feel free to put back or so on as my goal is to avoid having two packages that do the same thing (crossing fingers for my pull request). Thank you kindly
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Personally I'm not a fan of standard
either, but when I joined this project I didn't propose its removal because whether to have it or not is an entirely subjective question and there's no reason to get rid of it other than personal preference. If JP likes it, we should leave it in. The removal of standard would have to be a whole separate discussion and shouldn't just be removed because you don't like it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't say I didn't like it. I said I find it constrictive and unnecessary. I'm also harsh and direct and understandably my tone brings about a distaste for standard, which is untrue. I removed Express and other libraries that quite simply bloat this project and add weight to its overall size. This is a large version 2, big people changes. I can't wait to see ya'lls end choice as egos and opinions aside, just like Express 3 to 4, the hard breaking choices often are for the better.... Having more of a chat full conversation, put standard back in no bother me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let me try a less sharp reply : I use JavaScript over Java for how loose JavaScript is and how much faster I can move through it. I'm not in the least excited about Typescript, thankfully it's loosely typed. By my nature from experience, I'm not for the enforcement of making someone conform to a writing style. It's constrictive to development but yes has benefits from enforcement...... I'm providing a large revision to reload and with it comes my practices, which in this case is to free the project from writing style enforcement. As always, thank you kindly
@AckerApple thanks for all of your time spent here! Reviewing... |
I think it's fine to remove Node v0.10 and Node v0.12 support. Neither are supported by Node anymore. Plus this gives us some ES6 with Node v4. |
Thank you for recognizing my time spent. I've much appreciated the reload code and hope this pull request will continue its success into the future. Farewell new friend. |
… Exposed reload method for better manual reloading
After studying my personal implementation, I found the docs had a gap in step to use middleware. Please ensure latest code is used. If any further commits, would be only related to docs, code hints, or tests. |
A Java co-worker of mine said, for such a rewrite, I should have just forked and renamed. I wonder what ya'll think as well. It was my first thought just to release my fork on npm but I'm really for this contributing "mess" and not having two packages doing the same thing. Have a great week of success. Several of us on my end our eagerly waiting to see what happens. We are already installing from my fork and actively using starting today. |
Hello. We have been using this pull request quite aggressively. Currently reporting 28 Clones and 21 Unique cloners, for this pull request. Can anyone comment to this pull request being under active review? It's understood extra time could be due to the removal of the standard package. We are just looking for any communication on usage of this pull request, by the people that matter, the maintainers. Thank you in kind. |
Thanks @AckerApple for the reminder. @alallier is the lead maintainer of this repo now and the decision will be his and I fully support whatever he decides. I can say, that I'm doubtful this will be accepted as it's such a large change in one PR. Is there a way we could break this down? What are your thoughts on that @AckerApple? (btw, I'm speaking for myself on that last comment, it's still @alallier's decision). |
Sorry for the long delay @AckerApple (we are all busy!). There's a lot of good work in this PR but @jprichardson and I both agree it would make it easier to do this as a series of smaller PRs incrementally instead so we can discuss implementation details on a case by case basis. Let us know if you'd be interested in working on that with us. We love the enthusiasm you brought to the project and one way or another I'm sure a lot of this work will get merged in. |
The extra communication is throughly appreciated. It's very much understood the size of this revision requires much dedicated time, that is a lot to be asked of from out of no where. I have my own co-workers already on this pull request, saving us a lot of time when we have multi projects opening on the same port. This pull request is a no brainer, from where we sit. We've provided unit tests and a ton of documentation. We first adopted your code without a microscope, we can't expect you'd do the same I suppose. Well, you've closed the pull request. And almost like just closing an issue to have another issue closed, it has a rude taste about it. I think the same similar things could be said for my approach, as I Acker, would say in distaste of y'alls choices and decisions. So no words worth going down that road with. I'm going to publish this pull request on NPM and overtime keep an eye on yalls numbers versus mine cause I'm that kind of competive individual. Helps keep this motivating for us all. Maybe team up some other day down the line. Laugh about this post maybe. Farewell you good guys. |
I've spent so much time on this pull request. Please be slow to be dismissive and lets see if we can't have one of the largest pull request revision rewrites ever by open source software engineers.
I've been using reload for a long time. It's become perceived as behind the times and little wonky. Let's change that
V2 Includes
EADDRINUSE prompt
Added EADDRINUSE catcher that starts a cli-prompt when desired port is in use. Another port can be supplied to start server on another open port.
Multiple connections supported
Now multiple browsers and multiple web socket connections can be maintained
Watching Files
Watching files is more intuitive and actually available outside of just the CLI
Better Logging
Better verbose logging where an outside library can mandate how logging occurs
Weight loss
Removed a great amount of weight in dependencies. Package is far simpler to use and weighs far less
More ways to implement
Connect to any server, not just an express server. The non-cli reload.js is far more functional and easier to integrate into other projects.
Better Client Script Inclusion
reload package auto appends client script to all html requests
I'm going to be amazed if this is pulled, being its almost a complete rewrite. BUT, if this proves to be too big of an update to pull I guess I publish another npm package (we should avoid another package doing the same thing, only better :)
Thank you for your time
Edit by @alallier:
Instead of commenting on issues linking this PR, instead we would have the rather have the reverse (linking issues in the PR, rather than commenting on issues linking the PR). I'm just removing the comments and linking them here.
Closes #4, #6, #12, #25, and #42,