angular / ngSocket

WebSocket support for angular
Apache License 2.0
204 stars 25 forks source link

Remove cruft, configure bower for external use #6

Closed begriffs closed 10 years ago

begriffs commented 10 years ago

Hey Jeff, I wanted to open a pull request to start talking about how I can use this angular module in my project. I had trouble including it with bower so I made some tweaks that should help.

I was confused by dist.sh and the other source files laying around in the root. I removed them and configured the bower ignore section to omit tests etc when people include the project on the front end. I think the standard way to manage releases in bower is to tag commits on your repo that you feel are good. You don't have to run a dist script at certain times to capture a known good copy of the source.

Also the presence of all the launchers in the devDependencies are confusing. Looks like you only use Chrome. I left them in there because I thought perhaps you're planning to launch your demo app in the various browsers later to ensure portability.

Finally there's a strange line at src/ngSocket.js:110 which references ngWebSocketBackend. I couldn't see where that was defined. Not sure how it's working.

jeffbcross commented 10 years ago

Thanks @begriffs

dist.sh: this is a little convention for myself to neatly package up a project for publishing. This comes in handy because most projects have multiple source files that should be distributed as one file (or at least fewer files) for easier consumption. In this project right now, the usefulness of dist isn't apparent because all source is in one file, but I'll likely change that. I like having a bower package's published contents either in root or some kind of "build" or "dist" folder so the consumer of the package can infer that they're working with a packaged version of the code (implying that there's a fuller source to be discovered somewhere, and implying that the code is somewhat suitable for use).

The launchers in devDependencies are not needed, must just be an artifact from wherever I copied the package.json from.

re: ngSocket.js:110, that's surprising :). I'm sure it doesn't work; I may have committed hastily late at night.

I'll add some inline comments to the PR. BTW, in order to merge in your changes, I'd need you to sign the Google CLA: http://code.google.com/legal/individual-cla-v1.0.html

begriffs commented 10 years ago

How does this look? The end result should now live in dist/. I also submitted the CLA electronically.

After bower installing from my branch this is what gets included:

.
├── LICENSE
├── bower.json
├── dist
│   └── ngSocket.js
├── dist.sh
├── test-app.html
└── test-server.js

I wonder if we should add test-* and dist.sh to the bower ignore?

begriffs commented 10 years ago

@jeffbcross how do you like these changes? If you like it I can move on to getting the code to pass jshint and investigating how to fix the ngWebSocketBackend issue. I want to start using ngSocket for real in a personal project.