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

[Suggestion] Extracting the JS business logic to reduce duplication #104

Open
stof opened this issue Aug 29, 2014 · 5 comments
Open

[Suggestion] Extracting the JS business logic to reduce duplication #104

stof opened this issue Aug 29, 2014 · 5 comments

Comments

@stof
Copy link
Member

stof commented Aug 29, 2014

Currently, each method of the ZombieDriver needs to provide its JS business logic. This leads to code duplication. See for instance #103 which needs to be able to select a value for a radio button in 2 different methods.

My suggestion would be to move the business logic to a bunch of functions defined in the script running the zombie server itself. Then, the ZombieDriver would just have to use these functions when needed. This way, we could reuse some code by defining helper functions used by other functions, just like we do in MinkSelenium2Driver with a bunch of private methods in PHP for instance.
what do you think about this approach @aik099 ?

If we go this way, my suggestion would be to do the 1.2 release first (after fixing the tests), and then do the refactoring in a 1.3 release (to let us the time to ensure we don't introduce issues)

@aik099
Copy link
Member

aik099 commented Aug 30, 2014

My suggestion would be to move the business logic to a bunch of functions defined in the script running the zombie server itself. Then, the ZombieDriver would just have to use these functions when needed.

Moving JS code from PHP's HEREDocs, to corresponding Zombie process code which is written in JS is surely a good idea.

If we go this way, my suggestion would be to do the 1.2 release first

That is planned version, yes.

... (after fixing the tests) ...

I with the only problems resulting in failed tests were on driver's side. In reality some of them are in Zombie itself or libraries it uses.

... and then do the refactoring in a 1.3 release (to let us the time to ensure we don't introduce issues)

Since moving a code around is pure refactoring and not a feature introduction we don't really need to release 1.3 for it. The 1.2.1 will do just fine (since no public API or new functions are introduced).

@stof
Copy link
Member Author

stof commented Mar 5, 2016

After working on #162, I thought about what doing this work would involved.

Creating a full-featured node project for the driver would mean that we would have to deal with many files in our own code (probably one per driver method and a few more for the higher level codebase), and potentially some dependencies (at least a promise library as Node.js does not have native promise yet AFAIK and I would prefer using Promises than a callback hell). This would then make the driver harder to use:

  • if the PHP code is inside a phar, we would have to copy the whole node project to a temp folder, making it harder than today
  • if the node code has dependencies, we will have to rely on the user to install them from npm (as we do currently for zombie, which is our only dependency, and for which we need to support many version as we don't control the version constraint).

This would also be a BC break for people using the driver, as this would be a rewrite from scratch (configuration of the Server would have to be different).

So I thought about the ideal world:

  • the node code would be its own project (a pure node project available through npm). It would implement a complete API of the driver needs (and a clean way to tell the PHP code when it hits an error or an unsupported action) and use npm to manage its dependencies (allowing us to manage our own zombie constraint)
  • the PHP code would actually end up being totally decoupled from Zombie. This would end up as a RemoteDriver relying on a remote server to run commands (and optionally able to manage the process running this server, but the server could also be managed externally). The node project would be an implementation of such server based on zombie, but we could imagine implementing another one which would be used with the same PHP driver.

So I suggest investigating this as a whole new driver rather than as a v2 of ZombieDriver. We could then decide whether we want to keep maintaining the existing ZombieDriver or deprecate it in favor of the RemoteDriver. What do you think about this idea @aik099 ?

@aik099
Copy link
Member

aik099 commented Mar 5, 2016

Interesting. This new NPM project would be NodeJS sever which internally runs Zombie and in fact can be used by anybody who needs it (not only Mink driver).

the node code would be its own project (a pure node project available through npm). It would implement a complete API of the driver needs (and a clean way to tell the PHP code when it hits an error or an unsupported action) and use npm to manage its dependencies (allowing us to manage our own zombie constraint)

We can't guarantee, on PHP side, that a specific version of new NPM module is needed.

So I suggest investigating this as a whole new driver rather than as a v2 of ZombieDriver. We could then decide whether we want to keep maintaining the existing ZombieDriver or deprecate it in favor of the RemoteDriver.

Agreed.

@stof
Copy link
Member Author

stof commented Mar 5, 2016

We can't guarantee, on PHP side, that a specific version of new NPM module is needed.

I don't understand what you mean

@aik099
Copy link
Member

aik099 commented Mar 5, 2016

For example MinkRemoteZombieDriver (our new driver) in v1.0.0 will need MinkZombieServer 2.0.0 package from NPM to be installed.

How do we ensure using Composer alone, that the MinkZombieServer running on X port on remote (or even same) server is running correct version supports API that we need?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants