https://www.growthbook.io/ logo
Join Slack
Powered by
Hi <@U05TGREG4FM> I create a pull request, please ...
# sdk-flutter
c
Hi @calm-dog-24239 I create a pull request, please to review my changes maybe you have any concern. If Growthbook team approved for my changes, I will merge the PR. Thanks. https://github.com/growthbook/growthbook-flutter/pull/13 cc: @fresh-football-47124
🙌 1
c
Hi, @cool-apple-97384. Thank you for your pull request. We will review and test it.
c
Ok thanks @calm-dog-24239
🙌 1
c
We have time to test it, and I achieved that. When we launch the web platform and make a ‘fetch features’ call, we fall into the ‘listenAndRetry’. However, nothing happens, and the features are not fetched for the web platform.
c
Hi @calm-dog-24239 you mean my PR makes issue on the web platform?
c
Yes, we tested your pull request, and the issue still exists on the web. We are now looking into how we can resolve it.
c
How you test @calm-dog-24239? Use the example from the plugin?
Can you show the evidence? Because I successfully fetch the features on web
c
Yes, we use that. The problem was that on mobile it worked well, but on the web, it got stuck on consumeSseConnections. Okay, I will ask the Flutter developer for providing that.
c
Please comment on my PR @calm-dog-24239, so we can find others solution
Because I tested on web then running well use my PR
c
Ok, did you test it on SSE? Yesterday, our Flutter developer already added fixes for that. We hope to merge it today and create a new release version with that fix.
c
how to test it on SSE?
Your example doesn't support running on web
c
what example?
I mean SSE connection, we can you SSE connection or regular URL
c
I was improve the example, because existing doesn't support web https://github.com/growthbook/growthbook-flutter/tree/main/example
🙌 1
Can you test using this source?
c
No, I didn’t see your example. But when we test it, of course, we include the Web and test it on the web.
c
Because when we test on flutter web using my PR changes, our app show the value as expected on the mobile
c
Ok, we will do that.
c
Please test again
👍 1
c
Right now, it’s returning features, but there’s a very long delay on that listenAndRetry, and we enter this block, even though we shouldn’t if we don’t specify backgroundSync. Then our consumeSseConnections gets activated, but it should only be activated when backgroundSync is specified. Our developer has fixed this, and it will be included in the new release.
c
Yes, it's very long, because you guys set await for several seconds, that existing code
So, I can merge my changes @calm-dog-24239?
The issue just on waiting too long
If yes, please approve the PR
c
I wrote you that it has issue with backgroundSync, we need to fix it before merge
c
Okay, So I need to waiting these changes about 
backgroundSync
. Then, my PR need to merge with that changes. After merged, I can merge my PR @calm-dog-24239?
Another approach, growthbook team will contribute to my PR?
c
We can’t merge your PR because it didn’t work appropriately with backgroundSync. We need to fix these changes, because they will break the logic with the SSE connection and regular URL
c
Why you guys can't merge my PR? How about to contribute on my PR?
c
We can’t proceed without testing on SSE and the regular URL, without that, it could produce issues
👀 1
c
You guys can comment on the mistakes I made, let's fix them together
It's open source, right?
I didn't see any issue/comment or my PR, it's unfair for me
c
Yes, that’s also the way we want to do it. However, we need to add fixes and only after that, test and merge.
c
You look like you're going to solve this problem with your own PR and not take input from anyone else
If there are any errors in my PR, please comment
If there are shortcomings in my PR, let's contribute to each other's PR
c
Ok, we will write comments to your PR.
c
Ok thanks
This way if my PR has a little lack such as lacking just a few lines of code we can contribute together to the same PR
c
Just please understand that we can’t merge without testing and checking all of the functionality, because we are responsible for that repo.
c
Sure, takes your time for test all test case you have using my PR
🙌 1
I don't mind waiting if it's needed, therefore I don't immediately merge the changes I make, I appreciate that you are more entitled and responsible for the repository
🙌 1
c
Thank you for understanding.
c
You're welcome
I am waiting for the next update
👍 1
c
We added changes for refreshHandler and backgroundSync and merged your pull request. Thank you.
c
Ok thank you @calm-dog-24239 and team
🙌 1
I will waiting for the next published version
👍 1
c
Ok, thank you
11 Views
Open in Slack
PreviousNext
Powered by