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

Translate EarlGrey headers to Javascript calls #178

Merged
merged 35 commits into from
Aug 10, 2017

Conversation

DanielMSchmidt
Copy link
Contributor

@DanielMSchmidt DanielMSchmidt commented Jun 28, 2017

Plan

As migrating functionality from EarlGrey to detox is manual and therefore time consuming and error-prone process @rotemmiz came up with the idea of generating Javascript calls from EarlGrey itself.
The idea is to have EarlGrey as input, parse its header files to define the possible functions and result in a JS EarlGrey client that you may call that will then execute the equivalent Objective-C functions through the bridge.

This way we might be able to focus on real functionality and enable new contributors to not have to worry about the detox internals from the beginning.

Current State

  • Function Headers
  • Comments (see Feature Request: Comments in babel-types babel/babel#5897)
  • think about dynamically type checking (typeof ===) the arguments of the functions
  • find a solid way to test the output and / or verify automaically that the integration works
  • Function Bodies
  • add actual type to the typeof checks
  • add typeof checks for points
  • sanitize / group alike argument types
  • snakeCase function names
  • write a preambel stating that this is generated code
  • moved tap functions to generated code
  • document usage of script

GreyActions.h is transformed to detox/src/ios/EarlGrey/GreyActions.js

Update 1: @loganfsmyth hinted me in the right direction with the comments, so I got that one working. Still need to figure out how to add a space behind them, but if I can't find a babel option there still is good ol' str.replace(/\*\//g, '*/\n') ;)

@loganfsmyth
Copy link

You may want to consider passing your code through something like https://github.com/prettier/prettier to make the formatting nicer. Babel's toolchain by default does basic formatting, but making it super nicely readable isn't really one of our goals since most people don't read our output at all, and formatting the output nicely take longer.

@DanielMSchmidt
Copy link
Contributor Author

Great idea, that might make it much nicer 👍

@DanielMSchmidt
Copy link
Contributor Author

DanielMSchmidt commented Jun 28, 2017

@loganfsmyth I tried the on the output, but It seems to ignore the comments completely.

I will file an issue (prettier/prettier#2340) and maybe do a PR for that.
Maybe that use-case is a nice enhancement for them.

@DanielMSchmidt
Copy link
Contributor Author

I found a good first throw for the type checking and I will try to mimic the current behavior in terms of the return statement.

@DanielMSchmidt
Copy link
Contributor Author

For testing I would execute the transformation on a bunch of files and import the js file to then test their functionality, I think this is the easiest way

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.

Ho my, this is super neat!

demo.js Outdated
@return A GREYAction that performs a long press on an element.
*/static actionForLongPressAtPointduration(point, duration) {
if (typeof point === "object") throw new Error("point should be a object, but got " + point);
if (typeof duration === "number") throw new Error("duration should be a number, but got " + duration);
Copy link
Member

Choose a reason for hiding this comment

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

Maybe print typeof params instead of value

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great Idea, why don't we have both "duration should be a number, but got '42'(string)". Will add it to the todos

@rotemmiz
Copy link
Member

rotemmiz commented Jul 2, 2017

Re "exclude functions with unsupported argument types", we currently don't support passing Booleans in the objC invocation implementation.

@DanielMSchmidt
Copy link
Contributor Author

@rotemmiz Is there a reason that we don't support it? I thought about these out flagged functions.

@rotemmiz
Copy link
Member

rotemmiz commented Jul 2, 2017

It's a bug in the implementation, and it will be hard to fix since the code there is a bit messy. We need to refactor/rewrite it some day.

It does work on the Java implementation though ;)

@LeoNatan
Copy link
Contributor

LeoNatan commented Jul 5, 2017

@rotemmiz If you want, let's have a look at refactoring after I finish a milestone in the profiler / Instruments.

"test": "jest"
},
"author": "DanielMSchmidt <danielmschmidt92@gmail.com>",
"license": "ISC",
Copy link
Member

Choose a reason for hiding this comment

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

MIT please

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure 👍

@DanielMSchmidt DanielMSchmidt changed the title WIP: Translate EarlGrey headers to Javascript calls Translate EarlGrey headers to Javascript calls Aug 2, 2017
Copy link

@awitherow awitherow left a comment

Choose a reason for hiding this comment

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

Asides from some clarity in documentation and code structure this is really awesome. These opinions are just my own as well, take them or leave them 🙇 🙏

);
}

function capitalizeFirstLetter(string) {

Choose a reason for hiding this comment

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

it might be a good time to move capitalizeFirstLetter and methodNameToSnakeCase and sanitizeArgument either to another file that specifically is for helper methods, or at least organise them together so that the main functions in this file (createMethod, createClass, ...) are logically sorted for future reading of this file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea, the are all based around making the input more sane... Will do so 👍

isOneOf,
} = require('./type-checks');

function createClass(json) {

Choose a reason for hiding this comment

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

Would love to see some comment code on what these json structures look like, or at least references to them somewhere. It helps when visualising things moving through the functions 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will add comments like this 👍

function capitalizeFirstLetter(string) {
return string.charAt(0).toUpperCase() + string.slice(1);
}
function methodNameToSnakeCase(name) {

Choose a reason for hiding this comment

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

isThisSnakeCase?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

YesItIs 😂

t.identifier(name)
);

const isNumber = generateTypeCheck("number");

Choose a reason for hiding this comment

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

I am really stoked about this and would love to see this typecheck generator be a separate package I could use 👍 <3

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can extract that out, what would you search for if you would like to find it?

@DanielMSchmidt
Copy link
Contributor Author

@awitherow Extracted the guard clause generation into the babel-generate-guard-clauses.

@rotemmiz The build seems to be failing on master and the error looks the same as here, can this be merged?

@awitherow
Copy link

Nice @DanielMSchmidt the guard class generators are looking nice 👍

@rotemmiz rotemmiz merged commit e607fb3 into wix:master Aug 10, 2017
@DanielMSchmidt DanielMSchmidt deleted the code-generation branch August 10, 2017 07:01
@DanielMSchmidt
Copy link
Contributor Author

🎉 🎊 🎉 🎊🎉

@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.

5 participants