apache / incubator-weex

Apache Weex (Incubating)
https://weex.apache.org
Apache License 2.0
13.75k stars 1.81k forks source link

[iOS] The PR #2394 does not included in latest release version #2656

Open darkThanBlack opened 5 years ago

darkThanBlack commented 5 years ago

related: #2445 #2394

The code in WXSDKManager.m is still old version. This issue has appeared many times on my side. I can fix it by myself but still hope to be able to solve in the release version.  

janpio commented 5 years ago

That is very strange, as per git blame that line hasn't been changed for 2 years: https://github.com/wqyfavor/incubator-weex/blame/c72dc7fa570d5dcdb2d5f744f096ca12339106f3/ios/sdk/WeexSDK/Sources/Manager/WXSDKManager.m#L50

But https://github.com/apache/incubator-weex/commit/6dee825992ab2b8dc043b1bf82205a66adbec485#diff-ed3a1c92b451e932ba0afa5134c5ddd2L50 clearly shows this line getting added to master in the merge commit of #2394

https://github.com/apache/incubator-weex/commits/master?after=fdec2eeaefa9bfca4e46533049f1507a0b1ab9be+104 (commit history of master) clearly shows this commit actually happening on master (third commit from the bottom).

Something fishy is going on here or I am confused as hell.

janpio commented 5 years ago

Ok, forget what I wrote above: This was using a different fork of the repo, not Apache Weex!

janpio commented 5 years ago

I see the change in master https://github.com/apache/incubator-weex/blob/fdec2eeaefa9bfca4e46533049f1507a0b1ab9be/ios/sdk/WeexSDK/Sources/Manager/WXSDKManager.m#L50

If with "latest release version" you refer to 0.24.0, then you are correct: That was cut before that PR was merged.

The next release 0.26.0 will include the code changes you refer to.

darkThanBlack commented 5 years ago

OK, thank you for your contribution! BTW, I started tracking some cases about half a month ago and then opened the issue #2445, and found #2394 in the #2445 reply that was closed at 2019.05.07. In the past half a month I have received above 3 reports from my friends. In my opinion, _bridgeMgr will affect all weex plugins, the more noteworthy thing is many iOS developers using weex but I haven't found many issue or PR to point it out.

darkThanBlack commented 5 years ago

Let's take a look:

+ (WXBridgeManager *)bridgeMgr
{
    WXBridgeManager* result = [self sharedInstance].bridgeMgr;
    if (result == nil) {
        // devtool may invoke "unload" and set bridgeMgr to nil
        result = [[WXBridgeManager alloc] init];
        [self sharedInstance].bridgeMgr = result;
    }
    return result;
}

Recreate instance in singleton is uncommon code, particularly he also add a comment devtool may invoke "unload" and set bridgeMgr to nil. There must be some reason here. I have found weex playground source code in iOS have some serious problems when user change JS Debug Mode switch in weex dev-tool html, weex SDK will not refresh itself correctly, but I have not completely traced the reason. These codes may related to it.