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

Add pusher-js to npm package registry #84

Closed
grin opened this Issue Oct 13, 2014 · 58 comments

Comments

Projects
None yet
@grin

grin commented Oct 13, 2014

Would be helpful for those who use npm for front-end dependencies. Any plans?

@malte-wessel

This comment has been minimized.

malte-wessel commented Oct 15, 2014

+1

@pl

This comment has been minimized.

Member

pl commented Oct 16, 2014

Hi,
We're not planning to add pusher-js to npm, as it's not compatible with Node. I'd recommend using Bower, but I'm not sure if it's feasible for you.

@grin grin closed this Oct 16, 2014

@malte-wessel

This comment has been minimized.

malte-wessel commented Oct 16, 2014

It doesn't need to be Node compatible. Publishing it to npm is necessary to use the pusher-js with browserify

@m4tthumphrey

This comment has been minimized.

m4tthumphrey commented Nov 21, 2014

This really needs to be added.

+1

@grin

This comment has been minimized.

grin commented Dec 10, 2014

The npm site has been redesign recently and its new tagline is "npm is the package manager for javascript" ...I thought that it would be worth sharing it here as it shows the current trend of how npm is used by developers and its future direction.

So again 👍

@grin grin reopened this Dec 10, 2014

@leggetter

This comment has been minimized.

Contributor

leggetter commented Dec 10, 2014

Hey all - this is a discussion that we've also reopened internally. I'd like us to take a look at adding a UMD wrapper to pusher-js so that we can publish to NPM.

It may just be a simple build step but we need to do a bit of investigation and testing...

We'll update this issue as soon as things are a bit clearer. Thanks for the 👍s.

@ericdfields

This comment has been minimized.

ericdfields commented Dec 17, 2014

+1

@leggetter

This comment has been minimized.

Contributor

leggetter commented Jan 15, 2015

I've made a first attempt at supporting CommonJS includes by adding a UMD wrapper.

I've done a simple build with Browserify and tested Flash and HTTP fallback. I'd certainly appreciate it if others could test this. You can grab the built version of this here:
https://gist.github.com/leggetter/1c1658f0555a1a94180a

It's important to point out that:

  • this is just a quick attempt that I've put together and isn't suitable for production
  • that files for fallback dependencies e.g. JSON, Flash connections, HTTP connections are still loaded from the Pusher CDN for the released 2.2.3 version of pusher-js. This probably won't change if we release this.

However, this should let people include an use pusher-js via NPM and build with Browserify which I believe is the main objective.

If this UMD header appears to work we may be able to include it in a future pusher-js release and then we can start publishing it on npm as part of our release process. But there's a bit of testing that needs to be done before we do that and a decision about the version that gets the UMD header.

Please let me know if you have any feedback or questions about this.

@mwjackson

This comment has been minimized.

mwjackson commented Jan 19, 2015

@leggetter when are you planning to release this change?

@leggetter

This comment has been minimized.

Contributor

leggetter commented Jan 21, 2015

@mwjackson - I'm working towards making sure that I can run all the tests we have against the pusher-js version that's been built with a UMD wrapper.

I've got the unit tests running and passing but I need the integration test suite to be passing before I'd be happy to release it. Hopefully this can be done within a week.

@terebentina

This comment has been minimized.

terebentina commented Feb 25, 2015

+1 for the umd wrapper. The sooner the better, please

@leggetter

This comment has been minimized.

Contributor

leggetter commented Mar 3, 2015

Update: we have an internal version of this working and tested with Browserify. However, we've taken the opportunity to look into migrating the build process to more common node-based tooling. This includes testing, building and deployment processes.

We could release an early version, but there are some unanswered questions related to versioning and loading dependencies from a CDN. I'll try to speak to @pl about this over the next few days.

Sorry that it's taking so long to get this out.

@naorye

This comment has been minimized.

naorye commented Mar 15, 2015

Any news regarding this?

@leggetter

This comment has been minimized.

Contributor

leggetter commented Mar 15, 2015

The plan is for it to be in v3.0 of the library. It will contain:

  • UMD support
  • Remove Flash fallback (HTTP fallback is already in place and will be used instead)
  • Build and deployment tooling will be migrated from Ruby to Node

We'll try to get an alpha (a full version release may be quite a bit longer) of this out within the next three weeks. But it depends on what's prioritised and how much time we get to spend on library development. Fingers crossed an I will update this thread if there's any progress.

@juliankrispel

This comment has been minimized.

juliankrispel commented Mar 30, 2015

Would be great to have that 👍

@jhollingworth

This comment has been minimized.

jhollingworth commented Apr 1, 2015

👍

@zimbatm

This comment has been minimized.

Contributor

zimbatm commented Apr 1, 2015

Quick update, @pl has been working on moving the deployment tools to node but it's a bit more difficult that we hoped. We're pondering options

@mwjackson

This comment has been minimized.

mwjackson commented Apr 1, 2015

Do you need to convert your build tools to node to release a UMD wrapper?

@juliankrispel

This comment has been minimized.

juliankrispel commented Apr 1, 2015

@zimbatm just use UMD

@leggetter

This comment has been minimized.

Contributor

leggetter commented Apr 1, 2015

@mwjackson @juliankrispel We are using UMD. Specifically we're using https://github.com/umdjs/umd/blob/master/commonjsStrictGlobal.js

Right now the library internals have dependencies on a global Pusher object so this seemed the best choice.

Do you need to convert your build tools to node to release a UMD wrapper?

There are a number of reasons for the move to a different set of tooling.

  1. The current test suite works against unbuilt (non-concatenated) files. We obviously wanted to test the full built version and it seemed easier to do this using new tooling that to work with our own tooling which we have to maintain (there was a tricky use case here that I can't remember the full details of).
  2. We don't really want to fully maintain our own build tooling so fully using existing common tooling is a good move. If need be, we can simply contribute changes to existing projects.
  3. We want it to be easier for others to contribute to pusher-js; if this was an easy fix then we should of got a PR that we could merge without issue. But we haven't, so there's a problem there that we'd like to resolve. By moving to commonly used tooling where everybody can easily run the tests, perform builds etc. we're hoping things will improve.
  4. We want to remove Flash fallback and just use HTTP fallback instead. Doing this actually simplifies the build process. It was easier to add UMD support + new tooling without a Flash build dependency 🎉
  5. We're presently adding an improvement to the platform that requires a library change, so all of this was rolled into that process.

Right now we have a branch in a private repo that works.... but we can't test in IE6 or IE7 due to problems with the Node-based Jasime test runner 💩.

Hopefully that clarifies why this isn't as easy as top and tailing the existing script with a UMD header and footer. And hopefully we'll be able to solve the IE6 and IE7 testing problem (we do have customers who need this).

So, if anybody knows of a Gulp-based Jasmine 2.0 Test Runner that works in IE6+ please let us know

@juliankrispel

This comment has been minimized.

juliankrispel commented Apr 1, 2015

@leggetter thanks for shining a light on this

@zimbatm

This comment has been minimized.

Contributor

zimbatm commented Apr 2, 2015

Another solution would be to drop IE6 and IE7 support for the new lib. IE6+SSL is already broken because we removed SSLv3 support a while ago to prevent the POODLE attack.

@mwjackson

This comment has been minimized.

mwjackson commented Apr 2, 2015

I guess the question is whether those customers can continue to use the old version for their IE6/7 support. We don't have that requirement (thank god), so obviously the sooner you can get this out the better.

@zimbatm

This comment has been minimized.

Contributor

zimbatm commented Apr 2, 2015

And we could drop the JSON polyfill.

Just as a curiosity, why do you guys want npm packaging, to manage your dependencies with a single tool ?

@jhollingworth

This comment has been minimized.

jhollingworth commented Apr 2, 2015

We use browserify so would like to just npm install pusher-js. right now I've got to have a separate script file just for pusher and then include pusher in via a shim which is really ugly...

@mwjackson

This comment has been minimized.

mwjackson commented Apr 2, 2015

We have a similar situation to @jhollingworth, but using webpack.

@zimbatm

This comment has been minimized.

Contributor

zimbatm commented Apr 23, 2015

Alright. I've got a r3.0.0 branch with UMD and a couple of other things. Anyone wants to test it out ?

@marcbachmann

This comment has been minimized.

marcbachmann commented Apr 23, 2015

I'll try it.

@zimbatm

This comment has been minimized.

Contributor

zimbatm commented Apr 23, 2015

Thanks, for browser testing you can also go directly to http://test.pusher.com/?version=3.0.0 and play around with the client

@terebentina

This comment has been minimized.

terebentina commented Apr 24, 2015

I got the chance to play a bit with the new 3.0.0 branch today.
There is a problem here: https://github.com/pusher/pusher-js/blob/r3.0.0/dist/pusher.js#L23
['Pusher'] there means the library (pusher-js) depends on a package named "Pusher".
The line should read:

define([], function () {

I manually changed and tested and it works fine in latest ff and latest chrome on windows. Haven't got the chance to do more tests but will surely do in the following days.
Will submit a PR in a bit.

Fun fact: I did actually have a "pusher" module installed (the node.js client) and all hell broke lose when I tried to compile as it was trying to load that module to satisfy the dependency. Without the "pusher" module it would simply say "pusher module not found".

terebentina added a commit to terebentina/pusher-js that referenced this issue Apr 24, 2015

@zimbatm

This comment has been minimized.

Contributor

zimbatm commented Apr 24, 2015

Thanks @terebentina, I'll update src/build/umd_header.js to match what you said.

@zimbatm

This comment has been minimized.

Contributor

zimbatm commented Apr 24, 2015

I have made another build with the updated headers if you want to give it another try

@terebentina

This comment has been minimized.

terebentina commented Apr 24, 2015

Yep, ok, will test tomorrow.

@terebentina

This comment has been minimized.

terebentina commented Apr 27, 2015

Ok, this works fine now. For the record, I bundle my files with webpack. Maybe someone who uses browserify could test it too?
I tested with chrome and ff win latest, android 4.4 chrome and default browser and with mobile safari 7.0, no problems.

@marcbachmann

This comment has been minimized.

marcbachmann commented Apr 27, 2015

👍 I've just tested it using browserify. Works like before in Firefox & Chrome.

@juliankrispel

This comment has been minimized.

juliankrispel commented Apr 27, 2015

Awesome +1

On 27 Apr 2015, at 13:39, Marc Bachmann notifications@github.com wrote:

I've just tested it using browserify. Works like before in Firefox & Chrome.


Reply to this email directly or view it on GitHub.

@leggetter

This comment has been minimized.

Contributor

leggetter commented May 7, 2015

npm install pusher-js

🚀 🎆 😲 🎉 🎈

This is strictly still a release candidate, but I've used it a bit and it seems "all good" to me. I've also created a super-simple reference here really just to check things build and work as expected:
https://github.com/pusher/pusher-js-npm-basics

Please leave any feedback here.

Sorry this has taken SOOOOO long

@leggetter

This comment has been minimized.

Contributor

leggetter commented May 13, 2015

Since there hasn't been any further feedback and pusher-js is in NPM I'm going to close this issue.

Thanks to everybody that contributed. We'll officially announce the release pusher-js 3.0.0 shortly.

@leggetter leggetter closed this May 13, 2015

@DanielApt

This comment has been minimized.

DanielApt commented May 15, 2015

We're sadly seeing issues with webpack. @m4tthumphrey could you tell me what you did to get it working?

We get the following warning:

WARNING in ./~/pusher/~/request/lib/optional.js
Critical dependencies:
11:13-32 the request of a dependency is an expression
 @ ./~/pusher/~/request/lib/optional.js 11:13-32

Followed by many errors all related to request:

ERROR in ./~/pusher/~/request/request.js
Module not found: Error: Cannot resolve module 'net' in /Users/danielapt/Desktop/pusher/node_modules/pusher/node_modules/request
 @ ./~/pusher/~/request/request.js 36:10-24

ERROR in ./~/pusher/~/request/~/tunnel-agent/index.js
Module not found: Error: Cannot resolve module 'net' in /Users/danielapt/Desktop/pusher/node_modules/pusher/node_modules/request/node_modules/tunnel-agent
 @ ./~/pusher/~/request/~/tunnel-agent/index.js 3:10-24

ERROR in ./~/pusher/~/request/~/tunnel-agent/index.js
Module not found: Error: Cannot resolve module 'tls' in /Users/danielapt/Desktop/pusher/node_modules/pusher/node_modules/request/node_modules/tunnel-agent
 @ ./~/pusher/~/request/~/tunnel-agent/index.js 4:10-24

etc...

To reproduce please check out this repo:
https://github.com/DanielApt/webpack-pusher

@leggetter

This comment has been minimized.

Contributor

leggetter commented May 15, 2015

Sound like you are using "pusher". The NPM module for the browser library
is "pusher-js".
On 15 May 2015 3:17 pm, "DanielApt" notifications@github.com wrote:

We're sadly seeing issues with webpack. @m4tthumphrey
https://github.com/m4tthumphrey could you tell me what you did to get
it working?

We get the following warning:

WARNING in .//pusher//request/lib/optional.js
Critical dependencies:
11:13-32 the request of a dependency is an expression
@ .//pusher//request/lib/optional.js 11:13-32

Followed by many errors all related to request:

ERROR in .//pusher//request/request.js
Module not found: Error: Cannot resolve module 'net' in /Users/danielapt/Desktop/pusher/node_modules/pusher/node_modules/request
@ .//pusher//request/request.js 36:10-24

ERROR in .//pusher//request//tunnel-agent/index.js
Module not found: Error: Cannot resolve module 'net' in /Users/danielapt/Desktop/pusher/node_modules/pusher/node_modules/request/node_modules/tunnel-agent
@ ./
/pusher//request//tunnel-agent/index.js 3:10-24

ERROR in .//pusher//request//tunnel-agent/index.js
Module not found: Error: Cannot resolve module 'tls' in /Users/danielapt/Desktop/pusher/node_modules/pusher/node_modules/request/node_modules/tunnel-agent
@ ./
/pusher//request//tunnel-agent/index.js 4:10-24

etc...

To reproduce please check out this repo:
https://github.com/DanielApt/webpack-pusher


Reply to this email directly or view it on GitHub
#84 (comment).

@DanielApt

This comment has been minimized.

DanielApt commented May 15, 2015

Whoops… * facepalm *

@joshhornby

This comment has been minimized.

joshhornby commented Jul 3, 2015

I am trying to use the NPM package in a React application. I am importing the package like so:

import Pusher from "pusher-js";

But when I compile with webpack I get the following error:

node_modules/pusher-js/dist/pusher.js:1196
  if (!window.JSON) {
       ^
ReferenceError: window is not defined

Any ideas?

@DanielApt

This comment has been minimized.

DanielApt commented Jul 3, 2015

@joshhornby Use CommonJS syntax to import pusher-js:
var Pusher = require('pusher-js');

We've been successfully using pusher-js with webpack.

@joshhornby

This comment has been minimized.

joshhornby commented Jul 3, 2015

@DanielApt Thanks for the advice.

Still getting the same error about the window not being defined. Have you used this is a react application?

@DanielApt

This comment has been minimized.

DanielApt commented Jul 3, 2015

@joshhornby No, we haven't used it in combination with a react app. I'm a bit stumped on this one…

What is the target of your webpack project? Window being undefined would make me think it's set to node, instead it should be web. Maybe the target documentation helps you.

@leggetter

This comment has been minimized.

Contributor

leggetter commented Jul 6, 2015

@DanielApt I'd suggest going back to basics and working out where the problem is introduced. There's a very basic webpack example here:
https://github.com/pusher-community/pusher-js-npm-basics

How about starting there and building up to include react etc.

Or have you resolved the problem now?

@DanielApt

This comment has been minimized.

DanielApt commented Jul 7, 2015

@leggetter This is actually @joshhornby's issue, not mine. Anyways… @joshhornby I suggest you look at the above comment.

@joshhornby

This comment has been minimized.

joshhornby commented Jul 7, 2015

Thanks @leggetter will take a look.

@everblaze

This comment has been minimized.

everblaze commented Aug 21, 2015

was there any resolution to this error im getting the same..

@everblaze

This comment has been minimized.

everblaze commented Aug 21, 2015

Never mind worked out what the problem was to do with isomorphic application when using react. It was requiring the lib while on the server before hydrating to the client.

@coderholic

This comment has been minimized.

coderholic commented Mar 30, 2016

@everblaze @joshhornby how did you resolve this? I'm getting the same error with a react app.

@jackfranklin

This comment has been minimized.

Contributor

jackfranklin commented Apr 1, 2016

@coderholic can you replicate the bug in a GitHub repo that we can look at?

I'd recommend checking out Phil's comment above [https://github.com//issues/84#issuecomment-118828651] and going from there.

If you still have the problem also it might be worth opening a new issue; this issue is pretty long and not that easy to track problems in. A new issue will be way easier for all involved :)

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