Skip to content

Made axios work #36

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

Closed

Conversation

Adesin-fr
Copy link
Contributor

The error seems to come from the Connector not having a constructor, and maybe some other fixes I've done, don't remember fully... ;)

@leob
Copy link
Collaborator

leob commented Dec 2, 2024

The error seems to come from the Connector not having a constructor, and maybe some other fixes I've done, don't remember fully... ;)

@Adesin-fr Okay thanks, odd that the Connection thing would fix it because it was also complaining about other things not related to Connection, but okay, you made some other changes as well (e.g. the Typescript config) ...

But yes, ESLint seems to run successfully now in Github.

I'm going to merge this but then I'm not going to publish a new version right away - I'll first test it with our app as a "local dependency", and try to get the reconnect stuff working in our app.

I see that you pinned the echo dependency to a specific version, I think I'll want to set it to a higher version (the one we use in our app).

I'd also love to get rid of yarn and switch to npm at some point, but we should probably do that in a separate PR (not change too many things at the same time), but if it gives me too many headaches then i might dump yarn right away.

Once all of this works I'll publish a new version!

@leob
Copy link
Collaborator

leob commented Dec 7, 2024

The error seems to come from the Connector not having a constructor, and maybe some other fixes I've done, don't remember fully... ;)

@Adesin-fr When I pull down the code and I do an npm install or an npm run build, I'm still getting these warnings:

(!) Unresolved dependencies
https://rollupjs.org/guide/en/#warning-treating-module-as-external-dependency
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')
created ./dist/laravel-echo-api-gateway.js, ./dist/laravel-echo-api-gateway.common.js, ./dist/laravel-echo-api-gateway.iife.js in 1.1s

But in the end it did create the output JS files - so yeah, maybe it's okay - I'm going to check if the build works in Github ...

@Adesin-fr
Copy link
Contributor Author

Yeah I saw this.
Don't know why, but it seems axios version is pretty old, and ditching it for pure fetch calls would surely remove those errors !

@leob
Copy link
Collaborator

leob commented Dec 7, 2024

Yeah I saw this. Don't know why, but it seems axios version is pretty old, and ditching it for pure fetch calls would surely remove those errors !

@Adesin-fr We tried that already but that wasn't a big success, we reverted that ... no we're keeping Axios, I'm trying to get the import working now.

P.S. I'm closing this PR because I made a follow-up PR (#37) ... I'm also trying to replace yarn by npm, I find it pretty annoying to "mix" the two all the time, I just want npm only.

@leob
Copy link
Collaborator

leob commented Dec 7, 2024

I'm closing this PR because I made a follow-up PR (#37) ... I'm also trying to replace yarn by npm, I find it pretty annoying to "mix" the two all the time, I just want npm only.

@leob leob closed this Dec 7, 2024
@leob
Copy link
Collaborator

leob commented Dec 7, 2024

I'm closing this PR because I made a follow-up PR (#37) ... I'm also trying to replace yarn by npm, I find it pretty annoying to "mix" the two all the time, I just want npm only.

@Adesin-fr I took your PR, then used that to create a new PR, messed around with it, got rid of yarn and replaced it by npm, messed around a bit more, and now finally the build and the npm-publish seems successful - yay !

I'm now going to test it with our app, let's keep our fingers crossed ... :)

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.

2 participants