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

Use pusher-js 3.2.1 #10279

Merged
merged 1 commit into from Aug 25, 2016

Conversation

Projects
None yet
2 participants
@islemaster
Member

islemaster commented Aug 24, 2016

Adds pusher-js@3.2.1 to our project and uses it in NetSim. This replaces pusher-js 2.2, which wasn't available as an NPM package so we were loading it from the Pusher CDN via a script tag in _apps_depedencies.html.haml.

I was pleasantly surprised to find that we were not affected by any breaking API changes in the upgrade.

  • Pro: Removes one external dependency (though we still depend on Pusher's servers in other ways)
  • Pro: Removes the action-at-a-distance / global Pusher being provided through a script tag instead of via an import statement.
  • Pro: Brings more than a year of bugfixes in the pusher-js library into our project.
  • Con: Increases the size of our netsim bundle.
    • netsim.min.js grows by about 62k, from ~554k to ~616k
    • The new version is a lot bigger too: pusher.min.js v2.2 file we no longer download from Pusher's CDN is only 15.6k
    • Confirmed that the CDN pusher.min.js 3.2.1 is 61k.
    • Overall I'm not too concerned about this, but it's worth considering.
  • Con: Automated test coverage for Pusher integration isn't great, so any change includes the risk of irregular behavior at scale.

Tested by loading NetSim locally (I have my local machine configured to connect to my own Pusher account) and monitoring the live Pusher connections and messages dashboard while I sent messages in the simulator. Everything looks good!

Use pusher-js 3.2.1
Adds [`pusher-js@3.2.1`](https://github.com/pusher/pusher-js) to our project and uses it in NetSim. This replaces pusher-js 2.2, which wasn't available as an NPM package so we were loading it from the Pusher CDN via a script tag in _apps_depedencies.html.haml.

* Pro: Removes one external dependency (though we still depend on Pusher's servers in other ways)
* Pro: Removes the action-at-a-distance / global Pusher being provided through a script tag instead of via an `import` statement.
* Pro: Brings [more than a year of bugfixes](https://github.com/pusher/pusher-js/blob/master/CHANGELOG.markdown) in the `pusher-js` library into our project.
* Con: Increases the size of our netsim bundle.
* Con: Automated test coverage for Pusher integration isn't great, so any change includes the risk of irregular behavior at scale.

Tested by loading NetSim locally (I have my local machine configured to connect to my own Pusher account) and monitoring the live Pusher connections and messages dashboard while I sent messages in the simulator.  Everything looks good!
@davidsbailey

This comment has been minimized.

Show comment
Hide comment
@davidsbailey

davidsbailey Aug 25, 2016

Contributor

LGTM. Glad to have more things imported via npm!

Contributor

davidsbailey commented Aug 25, 2016

LGTM. Glad to have more things imported via npm!

@islemaster islemaster merged commit 3649a87 into staging Aug 25, 2016

2 checks passed

ci/circleci Your tests passed on CircleCI!
Details
hound No violations found. Woof!

@islemaster islemaster deleted the pusher-js-3.2.1 branch Aug 25, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment