search

minkphp / MinkZombieDriver

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

Support and test installation with npm v3 #168

Closed arithmetric closed 8 years ago

arithmetric commented 8 years ago

npm v3, the suggested version of npm for Node.js v5, introduced a "flat" module dependency structure. While npm v1 and v2 install the tough-cookie dependency of the zombie module under the zombie module itself (node_modules/zombie/node_modules/tough-cookie), npm v3 likely will not nest the tough-cookie module under zombie but install it at the same level (node_modules/zombie and node_modules/tough-cookie).

If zombie is installed with npm v3, then the mink-zombie-server.js script cannot find the tough-cookie module and the script fails with the error:

Error: Cannot find module 'zombie/node_modules/tough-cookie'
    at Function.Module._resolveFilename (module.js:325:15)
    at Function.Module._load (module.js:276:25)
    at Module.require (module.js:353:17)
    at require (internal/module.js:12:17)
    at Object.<anonymous> (/home/travis/build/arithmetric/MinkZombieDriver/bin/mink-zombie-server.js:4:13)
    at Module._compile (module.js:397:26)
    at Object.Module._extensions..js (module.js:404:10)
    at Module.load (module.js:343:32)
    at Function.Module._load (module.js:300:12)
    at Function.Module.runMain (module.js:429:10)
    at startup (node.js:139:18)
    at node.js:999:3
]

This error occurs due to the hard-coded path at: https://github.com/minkphp/MinkZombieDriver/blob/master/bin/mink-zombie-server.js#L4

This PR provides compatibility for installations with npm v1/v2 or v3 by trying to load tough-cookie from both paths, the npm v3 flat form first, then the legacy nested form.

Also, this error was not caught by the Travis tests because they were not configured to use the version of npm included with the version of Node.js used. This PR updates the Travis script to use the npm installed with Node.

This test run shows the error when using npm v3 without the code fix: https://travis-ci.org/arithmetric/MinkZombieDriver/jobs/121820946

aik099 commented 8 years ago

Nice find. While reviewing your changes I've revisited original problem that requested such a replacement of tough cookie code. It was a bug in tough cookie reported in https://github.com/SalesforceEng/tough-cookie/pull/13 and it was then (in 2014 year) fixed and released as v2.2.2, , v2.2.1, v2.2.0, v2.1.0, v2.0.0, v1.2.0, v1.1.0, v1.0.0, v0.13.0, v0.12.1, v0.12.0, v0.11.0, v0.10.0. I guess we assume that whoever is using Zombie now would use version of tough cookie with a fix.

With that said we can actually drop whole tough cookie module manipulate code all together.

Your code changes however are also good and maybe you can extract them into safe_npm_module(module_name) function that will do that for module with any name.

arithmetric commented 8 years ago

Thanks for the review @aik099. I've updated the code as you suggested to create a general function.

Also, it's good to know the background for this use of tough-cookie. I see that the code was introduced in 0653e0e9. Since Zombie 2 is required by MinkZombieDriver and Zombie 2 requires tough-cookie 0.12.0 or later, it seems like this code should no longer be needed. This can be addressed in a separate PR.

aik099 commented 8 years ago

In ideal world what I would like to know is exact version of Zombie where tough cookie has been fixed. Then we can execute that patching code only before that version + your protection against different npm versions.

arithmetric commented 8 years ago

It looks like Zombie v2.0.0 is the first release that requires a version of tough-cookie with the fix (tough-cookie v0.12.1 is required by Zombie v2.0.0). Zombie's previous release (1.4.1) required a broken version of tough-cookie (0.9.13).

aik099 commented 8 years ago

The integration with Zombie 1.4.x was broken for a long time and then I've decided to fix things and dropped Zombie 1.4.x compatibility in favor of any Zombie 2.x+ (at the time there 20+ alphas released) version.

Was correct tough cookie version used with Zombie 2.0.0 stable release and up?

arithmetric commented 8 years ago

@aik099 Yes, every version of Zombie from v2.0.0 and on requires tough-cookie 0.12 or later.

aik099 commented 8 years ago

Cool. We already have function for version comparison in JS. We can just place IF around patching code and that's it.

arithmetric commented 8 years ago

@aik099 Could adding the conditional for using the tough-cookie fix be handled in a separate issue/pull request?

As I look at this more, it will introduce some extra complexity, and I'm not sure my use case is testing it.

aik099 commented 8 years ago

Sure you can send additional PR if needed. I'll just wait for @stof to look at this before merging.

stof commented 8 years ago

If Zombie 2+ always comes with a fixed version of ToughCookie, I'm in favor of removing the monkey patching entirely

aik099 commented 8 years ago

If https://github.com/minkphp/MinkZombieDriver/blob/master/bin/mink-zombie-server.js#L102 line doesn't allow any unstable (e.g. alpha) versions of Zombie, then I'm ok with removing monkey patching code as well.

arithmetric commented 8 years ago

@aik099 I installed Zombie 2.0.0-alpha3 and confirmed that the version check you referenced throws an error preventing use of 2.0.0 alpha releases of Zombie.

@stof I submitted another PR that removes the tough-cookie monkey patch as an alternative to this PR: #170

aik099 commented 8 years ago

Cool.

stof commented 8 years ago

Closing in favor of #170