-
Notifications
You must be signed in to change notification settings - Fork 55
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
Webpack support + remove legacy compat libraries #321
Conversation
@@ -1,6 +1,6 @@ | |||
var JSONPTransport = (function() { | |||
var noop = function() {}; | |||
var _ = window.Ably._ = {}; | |||
var _ = Ably._ = {}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will break the jsonp transport unfortunately, it currently relies on there being a window.Ably
object.
yay. Great to know I added some value there :) |
e7e9cf5
to
2afa34c
Compare
Thanks Matt :) 👍 except for the jsonp issue. Not sure what the best solution is there. I've added commit 84ddef1 which fixes the issue, though is a bit ugly (it uses |
@@ -94,7 +105,7 @@ var JSONPTransport = (function() { | |||
params = this.params, | |||
self = this; | |||
|
|||
params.callback = 'Ably._._(' + id + ')'; | |||
params.callback = jsonpcb + '._(' + id + ')'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice. I had a niggling feeling the JSONP needed to be treated differently, but in the end skipped over it :/
Great. @SimonWoolf do you want to merge & publish? |
@SimonWoolf should we merge this? can you do it? |
Sorry, forgot about this |
@SimonWoolf this resolves #318 and #307
I'm yet to see what impact it has on the CI build, but from testing locally it seems to be working fine so far.
cc @paddybyers