minkphp / MinkZombieDriver

Zombie.js driver for Mink framework
41 stars 49 forks source link

Extract the node server script to a dedicated bin #162

Closed stof closed 8 years ago

stof commented 8 years ago

Instead of configuring the host, port and node paths by performing string replacements in the JS source code, the script now receives the host and port through environment variables, and the node paths are handled through the builtin NODE_PATH environment variable.

This change opens the way to being able to run the server manually separately (as it is now a working node script which can be launched separately), but also to a bigger refactoring of the driver to move more logic to the node script instead of sending it code to eval (see #104). Such refactoring will come in separate PRs though (and maybe not soon).

This PR should be fully backward compatible (except if people were getting the output of the protected method getServerScript to run their own process instead of letting our codebase build the process, which would then miss the env variables), but I don't see any reason to do it and it would be quite insane as this involves protected APIs)

Closes #161

stof commented 8 years ago

/cc @minkphp/core-team

aik099 commented 8 years ago

I presume existing ServerTest would cover that it still works?

stof commented 8 years ago

@aik099 the driver testsuite actually. This is the part running the zombie server

aik099 commented 8 years ago

@aik099 the driver testsuite actually. This is the part running the zombie server

Nice.

aik099 commented 8 years ago

That might be too late for that, but #!/usr/bin/node should say #!/usr/bin/env node instead. This way we'll be including node from PATH rather than specifically from /usr/bin/node.

stof commented 8 years ago

@aik099 you can send a PR to update it. But this is just a nice to have anyway. ZombieDriver itself does not rely on the shebang to run the script (Windows does not support them). I actually used the same shebang than the one used by npm itself.

aik099 commented 8 years ago

@aik099 you can send a PR to update it.

The #166 PR created.