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

Stop transpiling detox and detox-server, use sources and require node >=7.6 #404

Merged
merged 9 commits into from
Nov 21, 2017

Conversation

mrtnrst
Copy link
Contributor

@mrtnrst mrtnrst commented Nov 13, 2017

Overview

Remove deprecated package and updated to latest suggest package per Babel docs.

image

Discussion

I was noticing errors with a project that was not importing babel-preset-latest as a dev dependency. Rather than importing deprecated code I went ahead and updated to current logic.

Example:

detox test

would return
Couldn't find preset "latest" relative to directory "/Users/arista/***/node_modules/detox"

@dferguson08
Copy link

🙏 Really need this 🙏 Nice find/fix, @mrtnrst

@rotemmiz
Copy link
Member

What version of node do you guys use?
I am thinking of dropping Babel altogether, node 7.6 handles detox and async await with no need to transpile any code.

@mrtnrst
Copy link
Contributor Author

mrtnrst commented Nov 13, 2017

We are currently using v8.6 LTS. I thought for legacy support but unsure how far back you wanted to go.

@rotemmiz
Copy link
Member

263 days since Node 7.6 was released. Good time to say bye to Babel :)

@mrtnrst
Copy link
Contributor Author

mrtnrst commented Nov 13, 2017

I can open another PR to remove Babel if you aren't on it already. 🙇

@rotemmiz
Copy link
Member

yes, please do 🤗

@mrtnrst
Copy link
Contributor Author

mrtnrst commented Nov 14, 2017

@rotemmiz I removed the extra babel packages but kept the build. Also added a min version for node engines which will introduce a breaking change, let me know your thoughts.

@rotemmiz
Copy link
Member

This is not enough, I want to remove all trace of babel.

  1. remove babel in deps
  2. remove babel in builds (detox/build.sh, detox-server npm run build)
  3. change package.json main in both from lib/index.js to src/index.js

Copy link
Member

@rotemmiz rotemmiz left a comment

Choose a reason for hiding this comment

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

🤗

},
"scripts": {
"build": "BABEL_ENV=test babel src -d lib",
"start": "node lib/cli.js",
"build": "node src -d lib",
Copy link
Member

Choose a reason for hiding this comment

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

we can drop build script altogether

@@ -1,7 +1,6 @@
#!/bin/bash -e

echo -e "\nTranspiling JavaScript sources"
Copy link
Member

Choose a reason for hiding this comment

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

drop the echo

@rotemmiz rotemmiz changed the title Update Detox and Detox Server to use Babel-Preset-Env Stop transpiling detox and detox-server, use sources and require node >=7.6 Nov 15, 2017
@brunolemos
Copy link

Any quick way to point the package.json to this pull request?
Tried "detox": "wix/detox#404/head", but it gives an error Package "undefined@undefined" doesn't have a "name". because detox uses lerna and the root package.json is not the package I want.

@rotemmiz
Copy link
Member

hey @mrtnrst , any updates on your side ? If you need any help, please ping me.

@mrtnrst
Copy link
Contributor Author

mrtnrst commented Nov 19, 2017

@rotemmiz sorry I've had a busy week. Going to spend some time later today to try and get the CI issues resolved.

@mrtnrst
Copy link
Contributor Author

mrtnrst commented Nov 20, 2017

@rotemmiz so this was a lot easier than I originally thought! I just have to pay closer attention to my changes. 😁 🎉

@brunolemos
Copy link

brunolemos commented Nov 20, 2017

Tested the PR, it works if I run npm run e2e:ios inside the detox/test folder.

But when testing on a an empty react-native project with detox and it says this:
Couldn't find preset "es2015" relative to directory "/Users/brunolemos/Projects/react-native-css-in-js-benchmarks/node_modules/detox/node_modules/child-process-promise"

PS: It's probably because I'm using yarn link though.

EDIT: After running yarn inside the child-process-promise folder it works.

@rotemmiz
Copy link
Member

@mrtnrst , one last thing, in order to really test it works, let's point the test project to use Detox from main and not specifically from source.
https://github.com/wix/detox/blob/master/detox/test/e2e/init.js

change:

const detox = require('../../src/index');

to

const detox = require('detox');

Then, if it passes CI, we're good.

@rotemmiz rotemmiz merged commit c2bc4de into wix:master Nov 21, 2017
@rotemmiz
Copy link
Member

Thanks ! 🙏

@wix wix locked and limited conversation to collaborators Jul 23, 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.

4 participants