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

Create bundle for message-bus-ajax #234

Open
nic-lan opened this issue Sep 10, 2020 · 15 comments
Open

Create bundle for message-bus-ajax #234

nic-lan opened this issue Sep 10, 2020 · 15 comments

Comments

@nic-lan
Copy link

nic-lan commented Sep 10, 2020

I have installed message_bus by doing

yarn add message_bus

and now in my package.json i can see the package version installed "^2.0.3-beta.2"

in the js i do :

MessageBus.subscribe('/channel', (data) => {
    console.log(data);
  })

and in the console when a event is published to the channel i receive the following error:

VM40:2 Uncaught SyntaxError: Unexpected token | in JSON at position 129
    at JSON.parse (<anonymous>)
    at Object.success (message-bus.js:175)
    at XMLHttpRequest.xhr.onreadystatechange

If i understand correctly the issue is generated here

Screen Shot 2020-09-10 at 23 53 18

after some investigation i can see that the issue is message-bus.js:257

  options.success(xhr.responseText);

where

responseText: "[{"global_id":-1,"message_id":-1,"channel":"/__status","data":{"/channel":1}}]
↵|
↵[]
↵|
↵"

which does not look very good to me.

Note

Also the javascript in this repo is different from what i see in the package installed with yarn.

To make a test i just tried to copy-paste the original source of the javascript in the repo and it worked as expected so i guess that the fix here is just release the right version of the javascript client in the npm or wherever is stored

:-)

@nic-lan nic-lan changed the title Javascript Client [Javascript Client] Uncaught SyntaxError: Unexpected token Sep 10, 2020
@SamSaffron
Copy link
Member

This looks related to chunked encoding. What browser are you using? What does your setup look like.

Payload is correct in that, this is how chucked encoding payloads are transported.

@nic-lan
Copy link
Author

nic-lan commented Sep 11, 2020

brave Version 1.13.82 Chromium: 85.0.4183.83 (Official Build) (64-bit)

it is a

  • rails (6.0.3.3)
  • ruby 2.7.1p83
  • message_bus 2.0.3-beta.2 from npm
# config/initializers/message_bus.rb
Rails.application.config do |config|
  # See https://github.com/rails/rails/issues/26303#issuecomment-442894832
  CustomMessageBusMiddleware = Class.new(MessageBus::Rack::Middleware)
  config.middleware.delete(MessageBus::Rack::Middleware)
  config.middleware.insert_after(Warden::Manager, CustomMessageBusMiddleware)
end

MessageBus.configure(backend: :redis, url: "redis://#{ENV['REDIS_HOST']}:#{ENV['REDIS_PORT']}/0", namespace: ENV['REDIS_NAMESPACE'])

in the js client just

import MessageBus from 'message_bus'

MessageBus.start();
MessageBus.callbackInterval = 500;

MessageBus.subscribe('/channel', (data) => {
    console.log(data)
})
# config/puma.rb
...
on_worker_boot do
  MessageBus.after_fork
end
...

@nic-lan
Copy link
Author

nic-lan commented Sep 12, 2020

Any news about this ?

Note
Also the javascript in this repo is different from what i see in the package installed with yarn.
To make a test i just tried to copy-paste the original source of the javascript in the repo and it worked as expected so i guess that the fix here is just release the right version of the javascript client in the npm or wherever is stored

As i mentioned in the issue description, copying and pasting in my project the 2 files /assets/message-bus.js and /assets/message-bus-ajax.js which contain different code from the package in npm version 2.0.3-beta.2 fixed the issue I was facing.

Also the npm page shows that the last release was 3 years ago

Screen Shot 2020-09-12 at 13 53 55

At the same time the /assets/message-bus.js history shows that the last changes happened 1 month ago:

Screen Shot 2020-09-12 at 13 56 48

So my question here is:

is it possible that the npm package is somehow outdated or not in sync with the javascript files in this repo ?

@SamSaffron
Copy link
Member

Also the npm page shows that the last release was 3 years ago

Oh this is a concern, I am guessing you are talking about a completely different npm package to the one @eviltrout published.

@smikhalevski appears to own this thing and is not associated with this project

@SamSaffron
Copy link
Member

https://www.npmjs.com/package/message-bus-client is our official npm package.

@SamSaffron
Copy link
Member

closing this @nic-lan as you appear to be using an unofficial npm package.

@nic-lan
Copy link
Author

nic-lan commented Sep 16, 2020

Sorry @SamSaffron

it took me a bit longer to answer

I changed the package to the official https://www.npmjs.com/package/message-bus-client as you suggested but now i am facing another error.

In particular I don't have the JQuery installed in my app so i receive:

message-bus.js:233 Uncaught Error: Either jQuery or the ajax adapter must be loaded
    at longPoller (message-bus.js:233)
    at poll (message-bus.js:443)
    at eval (message-bus.js:429)
longPoller @ message-bus.js:233
poll @ message-bus.js:443
eval @ message-bus.js:429
setTimeout (async)
poll @ message-bus.js:427
start @ message-bus.js:464
eval @ index.js:21
...

That makes sense because i did not required /assets/message-bus-ajax.js.
I would like to do that but when i check the code delivered in the npm packge i cannot find it.

Can it be that the additional ajax adapter is missing and must be added to the package ?

@SamSaffron
Copy link
Member

This makes sense, yeah we should get message-bus-ajax into our bundle. (or make an optional bundle for it)

@SamSaffron SamSaffron reopened this Sep 16, 2020
@SamSaffron SamSaffron changed the title [Javascript Client] Uncaught SyntaxError: Unexpected token Create bundle for message-bus-ajax Sep 16, 2020
@nic-lan
Copy link
Author

nic-lan commented Sep 22, 2020

Hei hei
any news about this ?

@SamSaffron
Copy link
Member

@nic-lan we will get to this.

@eviltrout what are your thoughts here, just add the optional -ajax file here:

"jsnext:main": "assets/message-bus.js",
or should we make an extra package?

@eviltrout
Copy link
Contributor

For Discourse's usage we already have jQuery ajax, so I wouldn't want to include it in the jsnext package. It would probably be better to create a new npm module for it to add the ajax function.

Another alternative would be to swap it out for the fetch api which is present in most browsers now. That depends on what the supported browsers are for message-bus @SamSaffron

@SamSaffron
Copy link
Member

I think chunked encoding is pretty experimental in the fetch API https://web.dev/fetch-upload-streaming/

we could just use raw XMLHttp though and provide hooks so jQuery can override, not sure.

@eviltrout
Copy link
Contributor

In that case I think the second module on npm is the way to go.

@nic-lan
Copy link
Author

nic-lan commented Oct 13, 2020

hei hei...
any news about this ?

@SamSaffron
Copy link
Member

I hear you @nic-lan it is just this is so low on my priority list. Maybe @eviltrout can add to his list but I would not expect this to be done for at least a few more months.

For the time being can you just cut and paste the 20 lines of JS into your project?

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

No branches or pull requests

3 participants