Skip to content
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

When main.jsbundle newer than app.jsbundle time, load main.jsbundle #38

Merged
merged 1 commit into from
Nov 7, 2015

Conversation

qingfeng
Copy link
Contributor

@qingfeng qingfeng commented Nov 7, 2015

Please do not merge, this is a question I found, send pullrequest facilitate discussions

E.g:
user install 0.9.5,For example, using the react-native-keyboard event
user upgrade to 0.9.6,but, Still default read app.jsbundle, on a crash

So, I changed main.jsbundle update if the machine, the priority is read main.jsbundle

@msftclas
Copy link

msftclas commented Nov 7, 2015

Hi @qingfeng, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!

In order for us to evaluate and accept your PR, we ask that you sign a contribution license agreement. It's all electronic and will take just minutes. I promise there's no faxing. https://cla.microsoft.com.

TTYL, MSBOT;

@qingfeng
Copy link
Contributor Author

qingfeng commented Nov 7, 2015

@msftclas done

@lostintangent
Copy link
Member

We had actually planned to implement this behavior for the 1.1 update, but I forgot :/ the Cordova plugin already does this, so it would be great to get parity for React. Did you want to review this PR for merge?


case NSOrderedDescending: // date1が大きいとき
return jsCodeLocation;
}
return [[NSURL alloc] initFileURLWithPath:packageFile];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great. If you do want to merge this, I would make a couple of tweaks:

  1. Name the variables packageFileAttributes and packageDate instead of appFileAttribs and appDate. binaryFileAttributes and binaryDate probably also sound more obvious that we are refering to the file in the app store binary instead of mainFileAttribs and mainDate.
  2. Rename jsCodeLocation to binaryJsBundleUrl
  3. Replace lines 44-53 with a simple if-else statement, that way we save a few lines and make the code look cleaner.
if ([binaryDate compare:packageDate] == NSOrderedAscending) {
    // Return package file because it is newer than the app store binary's JS bundle
    return [[NSURL alloc] initFileURLWithPath:packageFile];
} else {
    return binaryJsBundleUrl;
}

@qingfeng
Copy link
Contributor Author

qingfeng commented Nov 7, 2015

@lostintangent @geof90 I want to know my idea is reasonable?

@geof90
Copy link
Contributor

geof90 commented Nov 7, 2015

@qingfeng yes it is :)

@qingfeng
Copy link
Contributor Author

qingfeng commented Nov 7, 2015

@geof90 @lostintangent I changed over, please review, thanks

@lostintangent
Copy link
Member

This change looks good to me.

@qingfeng could you check that you completed the entire workflow for the CLA? It should have added a new tag to this PR when you finished it. Thanks!

@qingfeng qingfeng changed the title [WIP]When main.jsbundle newer than app.jsbundle time, load main.jsbundle When main.jsbundle newer than app.jsbundle time, load main.jsbundle Nov 7, 2015
@msftclas
Copy link

msftclas commented Nov 7, 2015

@qingfeng, Thanks for signing the contribution license agreement so quickly! Actual humans will now validate the agreement and then evaluate the PR.

Thanks, MSBOT;

@qingfeng
Copy link
Contributor Author

qingfeng commented Nov 7, 2015

@lostintangent

You have successfully signed your document using DocuSign :)

@lostintangent
Copy link
Member

I just tested this and it works perfectly. Nice work @qingfeng! I'll merge this as soon as @geof90 signs off, and we can include this in the next release. I'll probably release a new version to NPM on Monday, since this is an important fix, and my other PR resolves a regression, so I'd like to get them out their quickly.

@geof90
Copy link
Contributor

geof90 commented Nov 7, 2015

LGTM

lostintangent added a commit that referenced this pull request Nov 7, 2015
When main.jsbundle newer than app.jsbundle time, load main.jsbundle
@lostintangent lostintangent merged commit 2705e42 into microsoft:master Nov 7, 2015
@qingfeng qingfeng deleted the patch-1 branch November 7, 2015 19:20
@qingfeng
Copy link
Contributor Author

qingfeng commented Nov 7, 2015

🍻

@lostintangent
Copy link
Member

Hey @qingfeng, would you be interested in doing a Skype call to discuss what you're up to? You've been so helpful so far and I'd love to understand what cool stuff you're doing :)

@qingfeng
Copy link
Contributor Author

qingfeng commented Nov 8, 2015

http://facebook.github.io/react-native/showcase.html
image

This is our company developed APP,Can you use email? ~ Skype, I may not be very convenient :)

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.

None yet

4 participants