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

Fix all build/test issues on master for android, detox & objc CI stages + update to Babel 7 #20854

Closed
wants to merge 24 commits into from

Conversation

Salakar
Copy link
Contributor

@Salakar Salakar commented Aug 25, 2018

This 'fixes' the following failing CI stages:

  • test_android
  • test_objc
  • test_detox_end_to_end

Fixes

Missing Buck Dependency

This fixes Android tests failing to build on master when running:

./scripts/run-android-local-unit-tests.sh

Without this master fails to build with the following error:

image

This commit appears to be the culprit: bce77c8 - Android support annotations imported but buck file was not updated.


JS Bundles not being transformed with the React Native Babel preset

There's an issue with building JS bundles - no Babel preset is being applied to RNTester when using local-cli which causes a syntax error on Flow types and on JSX syntax:

image

KEEP: To fix this I've added a new format babel.config.js file at the root which works for both jest (babelrc: false is set on options so does not interfere) and RNTester/local-cli bundled output (babel-preset-react-native). Additionally react-native-xcode.sh was updated to run Node from the project root - otherwise it never picks up the babel config and will fail to run with the same issues as before.

EDIT: Below is no longer relevant - all updated to now use the release version of Babel v7.0.0
REMOVE: Once babel-preset-react-native has been updated to use beta.56 the babel.config.js can be updated to just be the following and all the deps taken out the package.json again (except for RN preset):

This one is for reference and not merging as babel-preset-react-native needs it's Babel deps updating to v7.0.0-beta.56 to match metro's Babel dep version to fix this. To confirm it's building on these versions i've added them to the package.json for now.

// KEEP
module.exports = {
  presets: ['metro-react-native-babel-preset'],
  plugins: [
    '@babel/plugin-transform-runtime',
  ],
};

I would PR to babel-preset-react-native but I can no longer find it, doesn't seem to exist on GitHub anymore - it was in the root of the project before?
EDIT: this is now metro-react-native-babel-preset - and latest version is now added.


Test Plan:

  1. Ran the Android local unit test shell script (as above), tests have all passed, output:
    image

  2. Ran the jest tests (npm run test), tests have all passed, output:
    image

  3. Full CI passing 👀

Release Notes:

N/A - Internals

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Aug 25, 2018
@react-native-bot react-native-bot added Platform: Android Android applications. ✅Test Plan Missing Changelog This PR appears to be missing a changelog, or they are incorrectly formatted. labels Aug 25, 2018
@Salakar Salakar requested a review from hramos as a code owner August 25, 2018 22:57
@Salakar Salakar changed the title [android] Fix build on master for unit tests - missing buck config [android][bundler] Fix build issues on master for unit & integration tests Aug 25, 2018
@pull-bot
Copy link

pull-bot commented Aug 25, 2018

Warnings
⚠️

🔒 package.json - Changes were made to package.json. This will require a manual import by a Facebook employee.

⚠️

❗ Big PR - This PR is extremely unlikely to get reviewed because it touches 2352 lines.

Generated by 🚫 dangerJS

@Salakar Salakar changed the title [android][bundler] Fix build issues on master for unit & integration tests [android][bundler][wip] Fix build issues on master for unit & integration tests Aug 25, 2018
@Salakar Salakar changed the title [android][bundler][wip] Fix build issues on master for unit & integration tests [wip] Fix all build/test issues on master for android, detox & objc stages Aug 25, 2018
@Salakar Salakar changed the title [wip] Fix all build/test issues on master for android, detox & objc stages Fix all build/test issues on master for android, detox & objc CI stages Aug 26, 2018
package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
@kelset
Copy link
Contributor

kelset commented Aug 28, 2018

Hey @Salakar thanks for the PR! I have left some comments because I fear I may be missing some things here and there.

cc @hramos who may also want to check this before we merge

@Salakar
Copy link
Contributor Author

Salakar commented Aug 28, 2018

@kelset no worries!

I'd personally avoid merging this yet as is, sorry - as we don't really want all these extra Babel dev deps being added to the project - these will be gone shortly; once I add in "metro-react-native-babel-preset": "0.44.x" all the others can be removed - I will sort and add commits here to cleanup, give me a couple hours 👍

(I was unaware that the preset had moved to metro-react-native-babel-preset - which is why I added these extra dev deps in as a interim solution to waiting for babel-preset-react-native to be updated - which as you've pointed out is now part of metro packages - so I will sort)

…Babel 7 devDeps.

Additionally update metro dependencies to ^0.44.0 to keep Babel 7 versions consistent.
@Salakar
Copy link
Contributor Author

Salakar commented Aug 28, 2018

@kelset changes done and passing again.

I've switched it over to the metro-react-native-babel-preset preset at version ^0.44.0 and also updated the other metro* dependencies to match this version (internally these all use beta.56 of Babel 7).

All the other Babel dependencies have been removed as no longer required.

Notes


  • @babel/plugin-transform-runtime (and in the babel.config) is required otherwise the following error occurs on bundling:
    image

Potentially related PR: https://github.com/facebook/metro/pull/198/files


  • @babel/runtime is required to pair with @babel/plugin-transform-runtime otherwise the following error occurs:

image


Once metro has been updated to use Babel 7 the following diff can be applied:

--- a/package.json
+++ b/package.json
@@ -170,10 +170,10 @@
     "graceful-fs": "^4.1.3",
     "inquirer": "^3.0.6",
     "lodash": "^4.17.5",
-    "metro": "^0.44.0",
-    "metro-babel-register": "^0.44.0",
-    "metro-core": "^0.44.0",
-    "metro-memory-fs": "^0.44.0",
+    "metro": "^0.45.0",
+    "metro-babel-register": "^0.45.0",
+    "metro-core": "^0.45.0",
+    "metro-memory-fs": "^0.45.0",
     "mime": "^1.3.4",
     "minimist": "^1.2.0",
     "mkdirp": "^0.5.1",
@@ -202,12 +202,12 @@
     "yargs": "^9.0.0"
   },
   "devDependencies": {
-    "@babel/core": "7.0.0-beta.56",
-    "@babel/runtime": "7.0.0-beta.56",
-    "@babel/plugin-transform-runtime": "7.0.0-beta.56",
-    "metro-react-native-babel-preset": "^0.44.0",
+    "@babel/core": "^7.0.0",
+    "@babel/runtime": "^7.0.0",
+    "@babel/plugin-transform-runtime": "^7.0.0",
+    "metro-react-native-babel-preset": "^0.45.0",
     "async": "^2.4.0",
-    "babel-eslint": "9.0.0-beta.2",
+    "babel-eslint": "^9.0.0",
     "detox": "^8.0.0",
     "eslint": "5.1.0",
     "eslint-config-fb-strict": "22.1.0",

(babel-eslint included as this runs on a beta Babel 7 currently - updating to v9.0.0 will on ^7.0.0)

@Salakar
Copy link
Contributor Author

Salakar commented Aug 31, 2018

@kelset should be good to go now 🤞 EDIT - yep all good 🎉

I had to fix a few more things after updating from master; specifically, buck issues - there were redundant & duplicate dependencies defined on master that was breaking build/buck fetch.

Sorry for the delay - on a different Mac today so had to re-setup my environment 😫

@kelset
Copy link
Contributor

kelset commented Aug 31, 2018

No problem! Thanks for doing this 💪 I'll check with Mike, maybe we can even get this in for rc4 :)

@Salakar
Copy link
Contributor Author

Salakar commented Aug 31, 2018

@kelset no problem :) will also give RC4 a whirl this weekend and upgrade our tests suite/app for invertase/react-native-firebase to use it and see what happens 🔥

@hramos
Copy link
Contributor

hramos commented Aug 31, 2018

I just saw this PR - @Salakar thanks so much for working on this!

@facebook-github-bot facebook-github-bot added the Import Started This pull request has been imported. This does not imply the PR has been approved. label Aug 31, 2018
Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

hramos is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

.flowconfig Show resolved Hide resolved
jest/preprocessor.js Show resolved Hide resolved
@Salakar
Copy link
Contributor Author

Salakar commented Aug 31, 2018

Ignore last 2 commits sorry - I derped and merged my local master into my PR branch - I reverted immediately though so it's back to normal. 🙈

@facebook-github-bot
Copy link
Contributor

I tried to merge this pull request into the Facebook internal repo but some checks failed. To unblock yourself please check the following: Does this pull request pass all open source tests on GitHub? If not please fix those. Does the code still apply cleanly on top of GitHub master? If not can please rebase. In all other cases this means some internal test failed, for example a part of a fb app won't work with this pull request. I've added the Import Failed label to this pull request so it is easy for someone at fb to find the pull request and check what failed. If you don't see anyone comment in a few days feel free to comment mentioning one of the core contributors to the project so they get a notification.

@facebook-github-bot facebook-github-bot added Import Failed and removed Import Started This pull request has been imported. This does not imply the PR has been approved. labels Sep 1, 2018
@Salakar
Copy link
Contributor Author

Salakar commented Sep 1, 2018

My guess is it's this commit 10 hours ago: a8a63b1 😫 will sync up again shortly


EDIT: most likely was the NPM DNS/SSL issue that caused this to fail. Had no issues locally with this commit and all the other CI stages are running fine 👍

@Salakar
Copy link
Contributor Author

Salakar commented Sep 1, 2018

Right so I synced up to master and everything runs fine locally - I think the failure may be due to an ongoing NPM service outage at the moment:

image

Yarn installs are failing because of this - network connectivity issue on the build log: https://circleci.com/gh/facebook/react-native/51201?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link


EDIT: yep just the NPM outage issue - can re-run test_node10 once connectivity issue is resolved

@gengjiawen
Copy link
Contributor

@hramos The import failed, need to import again.

@Salakar Salakar changed the title Fix all build/test issues on master for android, detox & objc CI stages Fix all build/test issues on master for android, detox & objc CI stages + update to Babel 7 Sep 3, 2018
Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

hramos has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

hramos added a commit to hramos/react-native that referenced this pull request Sep 5, 2018
@Salakar
Copy link
Contributor Author

Salakar commented Sep 13, 2018

Will close this PR now as several commits have now been merged that are a subset of this PR.

Thanks all.

@Salakar Salakar closed this Sep 13, 2018
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. Missing Changelog This PR appears to be missing a changelog, or they are incorrectly formatted. Platform: Android Android applications.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants