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

Handling websocket disconnects #31

Merged
merged 7 commits into from
Nov 14, 2024
Merged

Handling websocket disconnects #31

merged 7 commits into from
Nov 14, 2024

Conversation

Adesin-fr
Copy link
Contributor

@Adesin-fr Adesin-fr commented Nov 10, 2024

This PR adds code for frontend to #2 handle connections issues : disconnections, not being able to connect, automatically reconnect...

It also adds Bref 2.0 as a supported version (NB: in Bref 2.0, serverless.yml syntax changed a bit. See documentation...)

@leob
Copy link
Collaborator

leob commented Nov 11, 2024

@Adesin-fr Thanks, will review/merge today!

@leob
Copy link
Collaborator

leob commented Nov 11, 2024

@Adesin-fr Okay, I browsed the code changes a bit (not extensively yet, will do that later) - do I understand correctly that, whenever the connection (socket) gets closed, it will automatically attempt to reopen (reconnect) itself? Would that mean that a connection could never be closed, even if we wanted to, or am I misunderstanding that?

@Adesin-fr
Copy link
Contributor Author

If you call the close() method of the broadcast driver, it will be gracefully closed.
And when the browser tab is closed, connections and JS code are cleaned too...

@leob
Copy link
Collaborator

leob commented Nov 11, 2024

@Adesin-fr Great, thank you ... I'm going to review/test and then merge this one today or tomorrow :)

@georgeboot
Copy link
Owner

Maybe remove the console.log() calls

@Adesin-fr
Copy link
Contributor Author

That's what I was thinking also.
Some were there before me, I might have forgot some of mines ;)
But for sure, it's too verbose ;)

Perhaps add a "debug" option that would allow to enable logging ??
Not sure how that should be handled TBH...

@Adesin-fr
Copy link
Contributor Author

Just removed logs, Only kept 2 logs that are triggered when :

  • subscribing on private channel
  • parsing a message as json

@leob
Copy link
Collaborator

leob commented Nov 12, 2024

@Adesin-fr I think it looks good, but the console.log messages (most of which are gone now) might be useful in some circumstances? @georgeboot What about the idea of a debug option as @Adesin-fr already mentioned?

@georgeboot
Copy link
Owner

Yeah debug sounds good

@Adesin-fr
Copy link
Contributor Author

Adesin-fr commented Nov 13, 2024

It seems this commit does the trick. At least on tests.
I still have to check that Echo doesn't filters the .debug property of the options object, but it doesn't seem !

PS : Making a last commit to update the readme also...

Edit: I confirm the debug option works : when not set, nothing is output to the console, and when set to true, logs happens ;)

@leob
Copy link
Collaborator

leob commented Nov 13, 2024

@Adesin-fr Thanks, nice one ... will review and then merge (if all looks fine) !

@Adesin-fr
Copy link
Contributor Author

Hi,

Will you merge this soon ?
I have a project where a machine needs to be connected 24/24 and I need this feature ;)

@leob
Copy link
Collaborator

leob commented Nov 14, 2024

Hi,

Will you merge this soon ? I have a project where a machine needs to be connected 24/24 and I need this feature ;)

@Adesin-fr Okay let me merge it tonight and then test it with our app, and I'll let you know ...

@leob leob merged commit a120008 into georgeboot:master Nov 14, 2024
2 of 4 checks passed
@Adesin-fr
Copy link
Contributor Author

Nice ;)
I think we must also update the version in package.json so npmjs will pick this update ?

@leob
Copy link
Collaborator

leob commented Nov 14, 2024

Nice ;) I think we must also update the version in package.json so npmjs will pick this update ?

@Adesin-fr Yes you're right, let me update the version ...

However, what I did is, I've created a new release (0.5.1) and clicked "publish", but publishing to npmjs fails - the Github Action logs says:

Run yarn publish --access public --tag latest
........................
error Couldn't publish package: "https://registry.npmjs.org/laravel-echo-api-gateway: Not found"

This is my 3rd attempt and previously I got a different "yarn" error, but now it just keeps saying "not found" ...

Before this last error I'm seeing a ton of warnings, but I don't know if that's the cause of the above or just "warnings":

npm WARN lifecycle The node binary used for scripts is /tmp/yarn--1731603326585-0.19912559614290437/node but npm is using /opt/hostedtoolcache/node/12.22.12/x64/bin/node itself

axios (imported by js-src/Websocket.ts)
(!) Missing global variable name
Use output.globals to specify browser global variable names corresponding to external modules
axios (guessing 'axios')

and so on ...

At some point I just deleted the 0.5.1 release and tag, redid the whole thing with 0.5.2 (not sure if that was actually a good idea), but now in the yarn publish --access public --tag latest run I'm seeing that it's still referencing 0.5.1 instead of 0.5.2 ...

Also in the Github Actions list I keep seeing "Merge branch 'Adesin-fr-master'" - that's a merge commit for the PR that I pushed an hour ago, but Github just insists on re-running that ... ? Ah I see what it does - before the "publish" action it's always running the build/test action on the latest git commit within that "tag" - I guess that makes sense ...

Bit of a mess though and I'm in "trial and error" mode right now - who has got the brilliant idea to "make this work" ? ;-)

@leob
Copy link
Collaborator

leob commented Nov 14, 2024

@Adesin-fr Okay yet another attempt - same result:

"error Couldn't publish package: "https://registry.npmjs.org/laravel-echo-api-gateway: Not found"
info Visit https://yarnpkg.com/en/docs/cli/publish for documentation about this command.
Error: Process completed with exit code 1."

and that's all, no other clues or info as to what the problem is ... I tried to google it but coming up empty!

I'm completely stuck, unless someone has a brilliant idea ... the only thing is that the preceding "test" action also fails (the jest run fails) - could it be that the "publish" action fails BECAUSE the "test" action (i.e. the Jest run) also fails?

I'll try (but not today, tomorrow):

  • to fix the failing Jest run

  • or if all else fails maybe we could dump Yarn and switch to Npm ... ? at least with Npm we'll probably get more "answers" when we google our issues ...

But I'm pretty much stuck right now, no idea what the problem is.

@leob
Copy link
Collaborator

leob commented Nov 14, 2024

@Adesin-fr Wait lol, the error was staring me in the face:

"(!) Unresolved dependencies
https://rollupjs.org/guide/en/#warning-treating-module-as-external-dependency
axios (imported by js-src/Websocket.ts)"

"Unresolved dependencies ... axios" - I guess this is what it's tripping over - this line maybe:

import axios from 'axios';

Should that actually be there ... let me try to remove that :)

@leob
Copy link
Collaborator

leob commented Nov 14, 2024

@Adesin-fr "Fixed" a number of things, deleted tag/release, created new one, "Unresolved dependencies ... axios" is gone now - however, stil getting this error in Github's "publish" action:

"error Couldn't publish package: "registry.npmjs.org/laravel-echo-api-gateway: Not found"

I'm getting a bit desperate now, running out of ideas ... will have a fresh look tomorrow.

@leob leob mentioned this pull request Nov 15, 2024
@leob
Copy link
Collaborator

leob commented Nov 15, 2024

@Adesin-fr Discussing with the other contributor on the other PR thread, maybe he has a clue ... there are still 2 things I could try, long shot but I'm out of ideas at the moment :)

Those two things are:

  • pass the --verbose flag to the yarn command, maybe it shows something which provides a hint

  • or maybe we can just dump yarn and switch to npm - I mean, does yarn now (in 2024) still have any advantages over npm ... ?

The second one sounds a bit drastic but it shouldn't be a big deal swapping yarn out for npm ... at least when npm gives an error during publishing it will likely be easier to google it as npm is much more "mainstrean" :)

P.S. by the way: the "test" action that precedes the "publish" action also has failures - the Jest run fails, as well as another job - but I don't believe that that should directly influence the "publish" action ...

@leob
Copy link
Collaborator

leob commented Nov 16, 2024

@georgeboot Are you able to help? I've now come to the conclusion that I'm probably unable to resolve this myself ...

So, what I'm trying to do is publish a new release, this one:

https://github.com/georgeboot/laravel-echo-api-gateway/releases/tag/0.5.1

but the Github "publish" action keeps failing at "yarn publish" (yarn publish --access public --tag latest) - see the logs:

https://github.com/georgeboot/laravel-echo-api-gateway/actions/runs/11869630146/job/33080163251

So it's unable to publish to npmjs ... I added --verbose to yarn publish to get more detail, and then it shows an HTTP 404 on the PUT command (see screenshot):

image

I was then able to google that, and found some pointers - this one:

https://stackoverflow.com/questions/39115101/getting-404-when-attempting-to-publish-new-package-to-npm

and this one:

npm/cli#1637

These make me believe that the most likely cause is there might be something wrong with the NPM token in our "Github secrets" - I mean this (npm/cli#1637 (comment)):

image

Are you able to check that? I don't think I have access to our "Github secrets" ... there might be yet other causes/reasons, but all of them seem related to auth/permission errors (the HTTP 404 error is misleading and very uninformative ...)

@georgeboot
Copy link
Owner

I think it should be fixed now.

https://www.npmjs.com/package/laravel-echo-api-gateway

The token was not expired but it was a legacy style token. Replaced it now with a fine grained one, that seemed to do the job.

@leob
Copy link
Collaborator

leob commented Nov 22, 2024

@georgeboot Thanks, that's brilliant - and I think the "publish" is done now (I guess you did it?) - because I tried to re-trigger the job and then I got You cannot publish over the previously published versions: 0.5.1." ...

Yes that seems to be the case: 0.5.1 • Public • Published 2 hours ago (on https://www.npmjs.com/package/laravel-echo-api-gateway)

All good then!

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.

3 participants