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

Fixed reload command line regression (Closes #133) #134

Merged
merged 1 commit into from
Aug 6, 2017

Conversation

alallier
Copy link
Owner

@alallier alallier commented Jul 30, 2017

  • Added mime as a dependency
  • Handle all types of files statically
  • Inject reload script tag on HTML files only
  • Created public/private return API so that reloadClientCode (among other things later on need be) could be hidden from the public return API

(Closes #133)

@AckerApple
Copy link

I only eye balled the code and I'm pretty sure this code does not allow for URL variables on any item.

I could be wrong

@alallier
Copy link
Owner Author

Should be good now

@AckerApple
Copy link

You don't have and most likely don't need:

  • Content-Length header
  • etag support
  • gzip support
  • or anywhere close to being a rfc 2616 compliant file streaming service

Sure super glued something that works. How much time have I saved you calling you out on such poor releases? Please listen, Express 3 to 4 dropped every included middleware EXCEPT node-static for a reason. Don't be a blind I-can-do-it-all. Be an open source team player. You make us look bad when you rogue it alone just forcing an http server to work.

https://www.npmjs.com/package/node-static

Good luck. Open up

@AckerApple
Copy link

Also I dont think your hand made server will work on Linux with case sensitive files... I could be wrong I could be wrong BUT write some damn tests at this point as you play with peoples minds breaking sh!t.

Learn from my unit tests, I even have this awesome html5Mode that support non-hash based routing which reload does not: https://github.com/AckerApple/reload/blob/master/test/index.js

@AckerApple
Copy link

AckerApple commented Jul 31, 2017

This is a more concise way to decode your urls to paths

var originalPathname = decodeURI(url.parse(req.url).pathname);

https://github.com/cloudhead/node-static/blob/master/lib/node-static.js#L60

That would cover things like file%20name.html

Also, you might be ok on Linux with node read file taking care of the case sensitivity. Had issues myself in past

@AckerApple
Copy link

I just reinvented the wheel and who woulda guessed, it spins. Now they have a wheel with HTTP2 protocol coming out and I gotta reinvent that on my wheel too.

How do you, as a reinventor yourself (with limited time), desire to upkeep with an already existing tool that has far more specific care and support?

Express was too big, you only needed to step down in dependencies. Not reinvent plus maintain

@alallier alallier force-pushed the fixReloadCommandLine branch 2 times, most recently from 70b10d3 to 0385f4f Compare July 31, 2017 22:12
@alallier
Copy link
Owner Author

alallier commented Jul 31, 2017

@AckerApple you're right I was overthinking the serve-static thing. I have now implemented the solution with serve-static

@AckerApple
Copy link

Great choice. Farewell good fella

* Added serve-statics, finalhandler, and url-parse as dependencies
* Handle all types of files statically with serve-static, (other than
HTML and reload client file
* Inject reload script tag on HTML files only
* Created public/private return API so that `reloadClientCode` (among other things later on need be) could be hidden from the public return API
@alallier alallier merged commit 65be95a into master Aug 6, 2017
@alallier alallier deleted the fixReloadCommandLine branch August 6, 2017 00:35
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