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

Updates from Wed 1 Jul #1839

Merged
merged 28 commits into from
Jul 2, 2015
Merged

Updates from Wed 1 Jul #1839

merged 28 commits into from
Jul 2, 2015

Conversation

a2
Copy link
Contributor

@a2 a2 commented Jul 1, 2015

No description provided.

johanneslumpe and others added 19 commits June 26, 2015 16:00
Summary:
This is an edited re-submission of facebook#1458 because I'm stupid.
Closes facebook#1497
Github Author: Johannes Lumpe <johannes@johanneslumpe.de>

Test Plan: Imported from GitHub, without a `Test Plan:` line.
Summary:
@public
Now that watchman perf issue was fixed we can enable watchman-based fs crawling which is faster than node.
This showed an existing issue with some files missing from the blacklist which I addressed.

Test Plan:
./fbrnios.sh run
click around and scroll all the apps
Summary:
Another Pull Request implementing the changes in issue facebook#468 - Enabled Packager to run on Windows
This change relates to the blacklist fixes. It includes the path conversion for blacklist and changes to the default watched directory.  It has no impact on Mac OSX.
Closes facebook#893
Github Author: Joe Wood <joewood>

Test Plan: Imported from GitHub, without a `Test Plan:` line.
Summary:
When `RCTGetExecutorID` was a static function in the header file, it would return nil when the app was running with ASan enabled even though directly calling `objc_getAssociatedObject(executor, RCTJavaScriptExecutorID)` returned the correct ID as an NSNumber. Moving this function into the .m file fixes this issue.

Closes facebook#1712
Github Author: James Ide <ide@jameside.com>

Test Plan:  Run the UIExplorer with ASan enabled in Xcode 7. Before this diff, the app would just hang since the executor was unable to read a valid ID and so it would bail out from running JS. With this diff the executor runs the JS and the UIExplorer works fine.
…tion.

Summary:
Should close this issue and successfully pass iTunes Connect validation.
Closes facebook#1722
Github Author: Matt Revell <mattrevell82@me.com>

Test Plan: Imported from GitHub, without a `Test Plan:` line.
Summary:
Fixes a crash due to the selector regex not knowing about the nullability annotations. Adds support for both the core annotations `__nullable` and `__nonnull` plus their shorthand counterparts `nullable` and `nonnull`.

Objective-C allows the shorthand versions only at the front of a parameter type declaration like `(nullable NSString *)` but the regex will pick up `(NSString * nullable)` too. This shouldn't cause any adverse effects and I left the code this way to keep the regex readable.

Fixes facebook#1795

Closes facebook#1796
Github Author: James Ide <ide@jameside.com>

Test Plan:
 Wrote a bridge method that uses a nullability annotation and verified that it didn't cause the app to crash:
```
RCT_EXPORT_METHOD(method:(nullable NSNumber *)reactTag)
{
}
```

Also added a nullable annotation to RCTTest.
…typechecking

This should fix tests.

Test Plan: Run Travis CI tests. Also run the movies app and verify that there are no invariant violations.
Summary:
This omits the devDependencies (e.g. test infra), which are intended only for people working on RN.

Part of facebook#1737.
Closes facebook#1803
Github Author: James Ide <ide@jameside.com>

Test Plan: Imported from GitHub, without a `Test Plan:` line.
@facebook-github-bot facebook-github-bot added GH Review: review-needed CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. labels Jul 1, 2015
@ide
Copy link
Contributor

ide commented Jul 1, 2015

Just a heads up that if the jest upgrade in a2@5552368 lands we'll need to accelerate the migration to io.js (tracking in #1737).

PRs to merge:
#1819 (fixes tests on master)
#1382 (fixes tests on io.js)
Both of these will have merge conflicts with this sync so either the conflicts need to be fixed on the FB side or I can rebase them after this sync is merged into master (and tests will be temporarily broken).

Updates the tests in small ways so they run on io.js with some updates:

 - The Cache test which relies on Promises uses `runAllImmediates` for modern versions of Node because bluebird uses `setImmediate` instead of `process.nextTick` for Node >0.10.

Test Plan: Run `npm test` with the latest version of jest.
@a2
Copy link
Contributor Author

a2 commented Jul 1, 2015

Part of #1819 and #1382 need to be landed internally so doing that now. Will then merge the rest into this PR. Thanks, @ide!

ide and others added 5 commits July 1, 2015 13:01
…typechecking

Summary:
This should fix tests.

Closes facebook#1819
Github Author: James Ide <ide@jameside.com>

Test Plan:  Run Travis CI tests. Also run the movies app and verify that there are no invariant violations.
Summary:
[This is a preview diff for getting RN's tests to pass with a future version of jest that supports io.js and other future versions of Node. This can be merged once the diff to update jest is merged upstream and published.]

Updates the tests in small ways so they run on io.js with two updates:

 - The Cache test which relies on Promises uses `runAllImmediates` for modern versions of Node because bluebird uses `setImmediate` instead of `process.nextTick` for Node >0.10.

Closes facebook#1382
Github Author: James Ide <ide@jameside.com>

Test Plan:  Run `npm test` with the latest version of jest.
… Update_Wed_1_Jul

Conflicts:
	package.json
@ide
Copy link
Contributor

ide commented Jul 1, 2015

@a2 I have some idea of what the js failure is... I can help look into it but it's easier for me to test against master. Do you want to go ahead and merge especially since tests are failing on master anyway?

@a2
Copy link
Contributor Author

a2 commented Jul 1, 2015

I didn't want to merge it yet but since it's a JS change, that's not really my area of expertise. I asked @amasad to look into it since it's getting late in London but if we can't figure it out tonight, I'll look into it more tomorrow.

@ide
Copy link
Contributor

ide commented Jul 1, 2015

OK.

I think the packager/react-packager/src/JSTransformer/__tests__/Cache-test.js failure might be fixed by adding jest.runAllTicks(); at the end above the runAllImmediates call.

@amasad
Copy link
Contributor

amasad commented Jul 1, 2015

Why was runAllTicks changed to runAllImmediates? That seems to be what the breakage is

@ide
Copy link
Contributor

ide commented Jul 1, 2015

Bluebird changes its scheduler to use setImmediate when running on Node 0.12 or newer: https://github.com/petkaantonov/bluebird/blob/634af0e27ff4faab62c6c5bfd105527abcf8b06e/src/schedule.js#L10. There's an update to .travis.yml to run iojs-v2 instead of Node 0.10, so the runAllImmediates call was added.

@amasad
Copy link
Contributor

amasad commented Jul 1, 2015

ah, but we stopped using bluebird. We're now using https://github.com/then/promise

@ide
Copy link
Contributor

ide commented Jul 1, 2015

Then I think what's happening is that the runAllTicks call was always needed, and now with Promise it might be OK to drop the runAllImmediates call.

Amjad Masad and others added 3 commits July 1, 2015 17:37
…tIOS`

Summary:
@public

The API and implementation of `shouldInjectAJAXHandler` is very opinionated, and it does not solve many of the use cases that we'd like to address.

Since  `shouldInjectAJAXHandler` is basically juts injecting JS to the web page, we should let developer inject whatever JS that address different issues that they want to fix.

Test Plan:
Test this snippet at <Playground />

```
<WebView
  url="http://www.facebook.com"
  injectedJavascriptIOS="document.body.style.border='solid 10px red'"
/>
```
@a2
Copy link
Contributor Author

a2 commented Jul 2, 2015

The e2e tests are failing on Travis but I ran them locally and they passed. Going to merge this and we can figure out what's happening on Travis later.

a2 added a commit that referenced this pull request Jul 2, 2015
@a2 a2 merged commit 1d1386e into facebook:master Jul 2, 2015
@a2 a2 deleted the Update_Wed_1_Jul branch July 2, 2015 11:15
@a2
Copy link
Contributor Author

a2 commented Jul 2, 2015

Okay so the builds are passing now but I didn't change anything. WTF Travis.

@ide
Copy link
Contributor

ide commented Jul 2, 2015

@a2, @amasad thanks for getting the tests to pass again =)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.