georgeboot / laravel-echo-api-gateway

Use Laravel Echo with API Gateway Websockets. Works with Bref.sh and Laravel Vapor.
MIT License
99 stars 23 forks source link

AWS SDK Update and workflow package version update #29

Closed michaelaguiar closed 3 months ago

georgeboot commented 3 months ago

Probably add a editorconfig file and make sure it's adhered to. Otherwise every maintainer's ide is going to 'fix' code styling on all files.

leob commented 3 months ago

Probably add a editorconfig file and make sure it's adhered to. Otherwise every maintainer's ide is going to 'fix' code styling on all files.

@georgeboot Yes I noticed as well that the git diff looks "huge" just because of different indentation/formatting - I might want to have a go at quickly "fixing" that before we merge this to master ...

@michaelaguiar I'm gonna change the indentation back to the way it was before, let's try not to mess with formatting when saving a file ...

michaelaguiar commented 3 months ago

Probably add a editorconfig file and make sure it's adhered to. Otherwise every maintainer's ide is going to 'fix' code styling on all files.

@georgeboot Yes I noticed as well that the git diff looks "huge" just because of different indentation/formatting - I might want to have a go at quickly "fixing" that before we merge this to master ...

@michaelaguiar I'm gonna change the indentation back to the way it was before, let's try not to mess with formatting when saving a file ...

Sorry about that! I can go head and get this fixed if you haven't started yet?

leob commented 3 months ago

Probably add a editorconfig file and make sure it's adhered to. Otherwise every maintainer's ide is going to 'fix' code styling on all files.

@georgeboot Yes I noticed as well that the git diff looks "huge" just because of different indentation/formatting - I might want to have a go at quickly "fixing" that before we merge this to master ... @michaelaguiar I'm gonna change the indentation back to the way it was before, let's try not to mess with formatting when saving a file ...

Sorry about that! I can go head and get this fixed if you haven't started yet?

@michaelaguiar Okay yes please do, thank you!

michaelaguiar commented 3 months ago

Ok just fixed the formatting. Looks like most of the diff now is composer.lock. Take a look!

I also agree with the editorconfig addition to enforce syntax styles.

leob commented 3 months ago

@michaelaguiar I'm now checking my own fork (because I'm seeing that for our app we're still using our fork, not the main repo) and I see that in the past I also tried to upgrade aws-sdk-php and I deleted token from the config as well - because (I assume) that's what the newer SDK required ...

I tested that and it didn't work, so I reverted both changes (the SDK upgrade and also the token deletion) ... but:

So I'm just gonna test it with this PR, and if it works for our app (with PHP 8) then we can switch our app to PHP 8, and from the fork to the main repo - that would definitely be progress!

michaelaguiar commented 3 months ago

Yea my deployments were failing because the newer AWS SDK version expected an array instead of a string for token if I remember correctly.

Sounds good. I believe I pulled over all changes to fix it, but let me know if you run into any issues.

leob commented 3 months ago

Yea my deployments were failing because the newer AWS SDK version expected an array instead of a string for token if I remember correctly.

Sounds good. I believe I pulled over all changes to fix it, but let me know if you run into any issues.

@michaelaguiar @georgeboot Woohoo, lo and behold it works, victory!

I pulled the PR down, messed a bit with our composer.json to load the dependency locally, then did an AWS/Bref deployment:

So we need to delete that "token" again (I'll do that) and then I'm gonna merge this PR !

leob commented 3 months ago

Yea my deployments were failing because the newer AWS SDK version expected an array instead of a string for token if I remember correctly. Sounds good. I believe I pulled over all changes to fix it, but let me know if you run into any issues.

@michaelaguiar @georgeboot Woohoo, lo and behold it works, victory!

I pulled the PR down, messed a bit with our composer.json to load the dependency locally, then did an AWS/Bref deployment:

  • first it complained that it needed PHP 8.1 - so I switched our Bref runtime from 7.3 to 8.1
  • then it complained that the AWS SDK didn't like the "token" - so I removed the token from the config file
  • and then I did a final deployment and yes BINGO it worked - and our app is now on PHP 8, which is a big win, and on a newer AWS SDK which is also nice :)

So we need to delete that "token" again (I'll do that) and then I'm gonna merge this PR !

@michaelaguiar No wait lol, seems I can't make this change, because the PR branch to be merged into master is within your forked repo, can't modify it ... :)

Can you make that change please, i.e. delete the line with "token" again from config/laravel-echo-api-gateway.php ? Then I'll merge this PR right away, because I've already verified that it works ...

P.S. websockets absolutely working like a CHARM - and all of that on top of the Bref PHP 8.1 runtime and new PHP AWS SDK !

michaelaguiar commented 3 months ago

Done!

I have one other contributor that has added some new functionality on my fork. I am going to review it and see if we want to get it merged in this repo.

leob commented 3 months ago

Done!

I have one other contributor that has added some new functionality on my fork. I am going to review it and see if we want to get it merged in this repo.

@michaelaguiar Thanks, I've merged the PR now! Yeah if the change from that collaborator looks useful then just create a PR for it and we'll check it out ...

@georgeboot Just a (probably) "dumb" question: we've now merged this PR, but do we need to increase the composer version somewhere so that people (e.g. myself, or Michael) can pull a new version of this dependency? I just assume we need to increase a version number somewhere but I'm not sure lol ... :)

georgeboot commented 3 months ago

You need to release a new version from GitHub ;-)

leob commented 3 months ago

You need to release a new version from GitHub ;-)

@georgeboot Ah right, that makes sense, of course this stuff needs to go somewhere in the Composer "registry" ... you want to do that, or you want me to attempt that? By the way I saw that a couple of CI jobs were failing now, not sure if that's serious or not ...

georgeboot commented 3 months ago

If you release on GitHub it will automatically sync to packagist.

I'll try to take a look at the CI jobs for this time.

leob commented 3 months ago

If you release on GitHub it will automatically sync to packagist.

I'll try to take a look at the CI jobs for this time.

@georgeboot Thanks, that might be helpful, I didn't really look at it yet, just noticed that there were errors ... can look at it next time but I'm off to bed now :)

P.S. message is pretty clear, it's trying to run "pest" but that depends on a Composer plugin which is currentlty 'blocked' - question is of course if something in the PR caused this, or it was already like this before the PR :)

leob commented 3 months ago

If you release on GitHub it will automatically sync to packagist. I'll try to take a look at the CI jobs for this time.

@georgeboot Thanks, that might be helpful, I didn't really look at it yet, just noticed that there were errors ... can look at it next time but I'm off to bed now :)

P.S. message is pretty clear, it's trying to run "pest" but that depends on a Composer plugin which is currentlty 'blocked' - question is of course if something in the PR caused this, or it was already like this before the PR :)

@michaelaguiar @georgeboot Fixed it, at least the "pest" run - AWS-SDK-PHP wants PHP 8.1 minimum, so I increased the PHP version in ci.yml - but Larastan still fails, we're using an old version which doesn't support 8.1 - I tried to upgrade it but that wasn't successful ...

i'll continue with that - will fix the Larastan issues and then publish to Packagist :)

leob commented 3 months ago

@michaelaguiar @georgeboot Released it to Packagist now - version 0.5.0 ! Did a quick sanity test with our app, looks good - we can now say goodbye our fork and use "the real thing" ...

(however - I saw a build error notification saying that the NPM package couldn't be published - but, the existing version 0.1.2 is still there - https://www.npmjs.com/package/laravel-echo-api-gateway ... will look at this "later")

P.S. have also got Larastan running now - it's flagging a number of issues such as declaring parameters mixed instead of array, etcetera, we can take care of that later ...

michaelaguiar commented 2 months ago

Awesome, thanks for getting that merged! When I have some time, I can take a Quick Look at the larastan warnings and see if I can quickly get them updated as well. Sounds like minor syntax warnings so far.