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

expose api that can be invoked by code #127

Closed
mrhooray opened this issue Mar 20, 2014 · 15 comments
Closed

expose api that can be invoked by code #127

mrhooray opened this issue Mar 20, 2014 · 15 comments

Comments

@mrhooray
Copy link

I'm thinking of writing a gulp plugin for mocha-phantomjs.

After looked at the code, it seams bin/mocha-phantomjs consists of two parts: cli and spawning phantomjs.

Is it a good idea to extract spawning phantomjs part from executable into lib as a library and let the executable just be a cli wrapper?

A temporary workaround I can think of is that let the gulp plugin invoke mocha-phantomjs as a child process and check output & exit code. However it's not as transparent as using a library and the plugin has to do options checking/handling which should be part of the library.

@nathanboktae
Copy link
Owner

The spawning of phantomjs isn't that interesting expecially of if you directly depend on phantomjs, which maybe we should (if we don't then your module can :) - then basically all you would duplicate is the spawnPhantomJs function which is just a simple usage of child_process.

What value add would your plugin bring? I haven't heard of gulp until you mentioned it. I am leery of all these JavaScript build systems and their plugins as most are glorified wrappers around a shell command.

@mrhooray
Copy link
Author

Why repeating second part of the executable in other modules that depends on mocha-phantomjs?

Yes, either a wrapper around command or invoking the code. That's what plugins do as a glue between build system and tasks.

Gulp uses streams of files and orchestrator to manage dependencies and achieve maximum concurrency. The plugin would let this runner be orchestrated by gulp rather than part in gulp and part with commands/scripts.

@nathanboktae
Copy link
Owner

Why repeating second part of the executable in other modules that depends on mocha-phantomjs?

Because most of that is finding phantomjs, and I'm saying it should be in a known location. Honestly mocha-phantomjs should have it as a peer dependency or direct dependency. @metaskills What do you think?

I work at a windows shop and I wrote a MSBuild task for mocha-phantomjs and I ended up directly calling phantomjs voa phantomjs lib/mocha-phantomjs.coffee ourtests.html <reporter> <config as json>. The fact we have a node.js wrapper is convenience, and that it is node is arbitrary (CasperJS uses python for their CLI for example).

You should go that route. You'll save a process fork in the process too.

@mrhooray
Copy link
Author

Thanks @nathanboktae

@metaskills
Copy link
Collaborator

Honestly mocha-phantomjs should have it as a peer dependency or
direct dependency. @metaskills What do you think?

Yes, I am moving in that direction now. Do you think a simple addition to package.json would do? What version spec?

@laurelnaiad
Copy link

@mrhooray -- I was about to start a gulp plugin for this, and in particular, I want to be able to pipe to gulp-istanbul and/or gulp-coverage. Have you created a repo for your work yet? I'd be interested in helping out (or just using what you're creating!) ...

@nathanboktae
Copy link
Owner

Do you think a simple addition to package.json would do? What version spec?

We need (get to) to also get rid of all the logic that looks for phantomjs as it will always be in ../node_modules/phantomjs/bin/phantomjs relative to bin/phantomjs

Probably a minor bump is fine as it is not breaking as it will pull in phantomjs.

So my main question was do we have it as a peer dependency or normal direct dependency? I was leaning to peer, which is basically how it acts now.

@laurelnaiad
Copy link

Peer. It's not small and there are other testing tools that should share it.

@mrhooray
Copy link
Author

How would a module require peer dependency (phantomjs in this case) during testing? I guess it should have a copy under devDependencies, right?

Considering potential conflict introduced by peer dependencies, shall we just use it as a direct dependency?

@nathanboktae
Copy link
Owner

I guess it should have a copy under devDependencies, right?

Yes.

There is a potential for conflicts, but peerDependencies was designed for these "plugin" packages where there might be multiple things plugging in to the same module that wouldn't be possible to do if each plugin package had their own copy. There are definitely use cases for direct usage of phantomjs, as well as mocha-phantomjs does run under it so it fits in this intended scenario.

@laurelnaiad
Copy link

In reviewing the node blog entry on the subject of peer dependencies , I might want to modify my statement, because that blog entry describes a plugin as something a bit different -- would there be other plugins that, during the running of phantomjs in which mocha-phantomjs is the invoker as a CLI, also care about the phantomjs api? I think the answer is no when mocha-phantomjs is invoked from the CLI.

If you invoke it as @nathanboktae does, phantomjs lib/mocha-phantomjs.coffee ourtests.html <reporter> <config as json>, then it seems to me that mocha-phantomjs is given no control over the phantomjs version, and all you can do is use a runtime check to determine whether phantomjs is a compatible version.

If you invoke it through mocha-phantomjs' bin, then it is a different story altogether. To me, this suggests that the right approach is a peerDependency from the mocha-phantomjs library, and a different npm package, you could call it mocha-phantomjs-cli, that has the phantomjs dependency and the mocha-phantomjs library dependency.

Does that make sense to anyone?

@nathanboktae
Copy link
Owner

Does that make sense to anyone?

I've been thinking about this for a few days and yes it makes sense, but a couple of things. First I'd name the packages 'mocha-phantomjs' and 'mocha-phantomjs-core' for backward compatibility for existing users. I'd also be concerned about issue tracking - the two have a pretty heavy contract between each other. As more build plugins come out though, this would help. Its also a decent amount of work.

Thoughts @metaskillz ?

@metaskills
Copy link
Collaborator

Hey Nathan, I really do not have much feelings for this one way or the other. If the work is solid and you think it is a good idea. I'll defer to y'alls judgement.

@nathanboktae
Copy link
Owner

I finally started working on this: nathanboktae/mocha-phantomjs-core

@nathanboktae
Copy link
Owner

nathanboktae/mocha-phantomjs-core is ready for consumption now. Thanks all for your feedback.

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

No branches or pull requests

4 participants