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

feat(app:socket.io): build socket.io into vendor.js #455

Merged

Conversation

kingcody
Copy link
Member

Changes:

  • Move socket.io script tag inside vendor.js build
  • Add node_modules/socket.io/node_modules to vendor script locations
  • Change socket.io path to /socket.io-client in serve and client
  • Will not serve socket.io.js when server is ran in production env

@DaftMonk
Copy link
Member

grunt-changelog can't parse commits with colons in the scope i.e: (app:socket.io)

Could you change the commit message?

@kingcody
Copy link
Member Author

Doh! Sorry, will do.

One quick thought...
I use a build path of node_modules/socket.io/node_modules because socket.io requires socket.io-client and everything lines up nicely. My only addition would be to include socket.io-client in out package.json and use a build dir of node_modules instead. Reason being that if socket.io changed the order in which they included socket.io-client, our build would most likely break. But if we require it then we know it will be in top level node_modules and most likely npm will skip it as dep for socket.io since it was installed by a parent dep.

@DaftMonk
Copy link
Member

👍 Sounds good.

Changes:
- Move socket.io script tag inside vendor.js build
- Add `node_modules` to vendor script locations
- Change socket.io path to `/socket.io-client` in serve and client
- Will not serve socket.io.js when server is ran in `production` env
- Add `socket.io-client` to package.json for a more consistent build
@kingcody
Copy link
Member Author

@DaftMonk, in client/index.html the opening <script> for the google analytics and the script body itself are indented one space too many. Would you mind fixing that? Or should I add it to this PR?

DaftMonk added a commit that referenced this pull request Aug 17, 2014
feat(app:socket.io): build socket.io into vendor.js
@DaftMonk DaftMonk merged commit ec29184 into angular-fullstack:master Aug 17, 2014
@DaftMonk
Copy link
Member

Thats ok, I got it.

@kingcody kingcody deleted the feature/baked-in-socket.io branch August 17, 2014 08:31
<!-- bower:js -->
<!-- endbower -->
<% if(filters.socketio) { %><script src="socket.io-client/socket.io.js"></script><% } %>
Copy link
Contributor

Choose a reason for hiding this comment

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

@kingcody What happens, here? I'm confused.

Copy link
Member Author

Choose a reason for hiding this comment

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

We moved the socket.io script tag inside the usemin build block so it will get included in vendor.js. We add node_modules to the search glob for the build, and change the path to socket.io-client/... since we install socket.io-client as a dependency. That way the paths all line up nicely and we still use the same script that socket.io would serve.

Copy link
Contributor

Choose a reason for hiding this comment

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

ok, thank you :)

Copy link
Member Author

Choose a reason for hiding this comment

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

If you take a look at the server and client scripts where we setup socket.io, you should see where we've adjusted the paths to match.

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.

4 participants