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

Time for Version 2 #62

Closed
wants to merge 29 commits into from
Closed

Time for Version 2 #62

wants to merge 29 commits into from

Conversation

AckerApple
Copy link

@AckerApple AckerApple commented Feb 18, 2017

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,

@AckerApple
Copy link
Author

AckerApple commented Feb 18, 2017

I am unsure how to proceed with travis-cli failing checks. I am completely open to collaborate on this by any means possible.

@AckerApple
Copy link
Author

Passing travis now. Little painful. Lost NodeJs 0.10 support. Added documentation

@AckerApple AckerApple mentioned this pull request Feb 18, 2017
package.json Outdated
@@ -26,7 +26,6 @@
"ws": "~1.1.1"
},
"devDependencies": {
"standard": "^7.1.2"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why?

Copy link
Author

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

Copy link
Owner

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.

Copy link
Author

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.

Copy link
Author

@AckerApple AckerApple Feb 19, 2017

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

@jprichardson
Copy link
Collaborator

@AckerApple thanks for all of your time spent here! Reviewing...

@jprichardson
Copy link
Collaborator

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.

@AckerApple
Copy link
Author

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.

@AckerApple
Copy link
Author

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.

@AckerApple
Copy link
Author

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.

@AckerApple
Copy link
Author

AckerApple commented Mar 2, 2017

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.

@jprichardson
Copy link
Collaborator

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).

@alallier
Copy link
Owner

alallier commented Mar 4, 2017

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.

@alallier alallier closed this Mar 4, 2017
@AckerApple
Copy link
Author

AckerApple commented Mar 5, 2017

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.

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.

3 participants