Skip to content
This repository has been archived by the owner on Oct 2, 2021. It is now read-only.

Fixes for HCP errors #48

Closed
wants to merge 11 commits into from
Closed

Fixes for HCP errors #48

wants to merge 11 commits into from

Conversation

KoenLav
Copy link

@KoenLav KoenLav commented Jan 22, 2018

Contains changes made by @bmanturner aiming to fix HCP issues.

@KoenLav
Copy link
Author

KoenLav commented Jan 22, 2018

@bmanturner could you (further) clarify your changes so they can maybe be merged back into this repository?

@wojtkowiak
Copy link
Contributor

Most of those changes are from here:
matdurand@ef7b999
(credits @matdurand)

First is the blacklist retry: matdurand@ef7b999#diff-5a6122e074f120eb07eb0a3298e331d8R71 (for both ios/android) - it basically allows to do one retry before it blacklists the version. It would be nice if we could specify how many times it should be retried before getting blacklisted.

Second is the the method that allows you to change to the pending version matdurand@ef7b999#diff-e9a5d10fe686c1ff2b69a5030c60c165R203
From my research it seems to be safe to do that in the Reload.onReload and it is far better than switching when the onReset is fired in the webview which is the current mechanism. This fixes this issue which I also encountered (#4). However calling it would have to baked deeper in the Reload after all the callbacks from onReload are fired and we know we are doing a reload. Now if you add this as callback to onReload there is chance a callback that would be fired after would prevent HCP which is not good,

More on those changes made by @matdurand here meteor/meteor#8063

Maybe it would be better to extract those to separate PRs. @abernix let me know. I can confirm that those combined with the other fixes solved almost all issues with HCP I did experience.

@hwillson
Copy link
Contributor

hwillson commented Feb 8, 2018

Just dropping a reference - this PR will fix meteor/meteor#8063.

@KoenLav
Copy link
Author

KoenLav commented Feb 10, 2018

@wojtkowiak could you create separate PRs which solve the issues you are experiencing?

These would probably be easier to review than this one.

@menelike
Copy link

menelike commented Jul 5, 2018

We had to use this in production together with meteor/meteor#8063 (comment) and after several months we did not have one single user with an outdated bundle version. We would have been doomed without this PR, thanks a lot @KoenLav

IMHO this issue should gain more attention from the maintainers.

@KoenLav
Copy link
Author

KoenLav commented Jul 6, 2018

@menelike thanks, good to see it resolves your issues, but I guess this requires some more work before it can be merged.

@wojtkowiak first of all enjoy your holiday!

But after, maybe you have some time to revisit this?

@abernix @hwillson I agree with @menelike; we're almost there, let's make it happen :)

@bmanturner
Copy link

@KoenLav Sorry for the delay. I'll check my notification settings.

@wojtkowiak Was right in his accreditation. This was not my own work, and my git-fu isn't up to par enough to cherry pick his work. Sorry for the confusion.

@KoenLav
Copy link
Author

KoenLav commented Jul 31, 2018

@wojtkowiak do you think this PR should be closed in favor of a certain changeset in your (forked) repo?

This was referenced Sep 13, 2018
@KoenLav
Copy link
Author

KoenLav commented Oct 8, 2018

@benjamn @abernix we've been using these changes, which @wojtkowiak presented in separate PRs recently, in production and haven't seen any more errors.

Closing this PR, please do take a look at the others!

@KoenLav KoenLav closed this Oct 8, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants