mapbox / mapbox/node-pre-gyp

vNext - update codebase & increase maintainability

Open
#392 3 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

Dominant language
JavaScript
Stars
1.2k
Forks
271
Avg merge
23h 40m
Merged PRs (30d)
2

Description

#373 has been an interesting dive into the architecture of node-pre-gyp. I started by trying to mock the publish commands as such:

```js
var NPG = require('../lib/node-pre-gyp');
var npg = new NPG.Run();
npg.commands['publish']({clang: 1}, function(err) { ... });
```

This results in the following error:

```
Error: node-pre-gyp package.json is not node-pre-gyp ready:
package.json must declare these properties:
binary.module_name
binary.module_path
binary.host
```

After some digging, I thought we could spoof the directory by passing in `--director=/path/to/app1` when setting up the `npg` variable with the `Run()` command or using the `npg.parseArvg()` method. These didn't seem to do it, and then I realized why - the setup (versioning) code run [from within lib/publish.js](https://github.com/mapbox/node-pre-gyp/blob/4d88e052da37a71afa3407da1fccf340fe93148a/lib/publish.js#L19) uses the CWD instead of taking a set of objects as options. The CWD set up prior with `process.cwd()`, which means scripts like publish.js are dependent on other scripts running first in order to work, which complicates writing readable unit tests.

This is going to be a difficult hurdle to work around, which leads me to a blue-sky suggestion: **perhaps now is the time to rewrite/organize node-pre-gyp's code.** The hardest thing about writing tests for the library is that all methods must be executed in a child process rather than as direct node.js methods. This means we cannot mock anything, and have to rely on our tests to access external resources, which is an additional spot for failure.

This would allow us to write tests like this:

```js
const setupOpts = {
package_json: '/path/to/package.json'
};

const npg = new NPG(setupOpts);
npg.publish(publishOptions, callback);
```

I think we could do the rewrite without breaking changes to the CLI interface at all. Some pros/cons of doing this:

Pros:

* update to ES6 syntax
* make code more testable/mockable
* we can document the Node.js interface rather than 100% CLI interface (not sure if this is helpful outside of writing unit tests)
* use an opportunity to increase maintainers of the library (lower vacation factor)
* move system/architecture business logic into the library, rather than on the bin CLI level
* more maintainers = faster response times to issues
* gets us closer to a 1.x
* opportunity to drop support for specific Node/NPM versions

Cons:

* hard, takes time
* likely I'm missing some additional context of why `node_pre_gyp.Run()` is set up the way it is, so perhaps this is impossible?
* not critical, as far as most pressing issues in node-pre-gyp

cc @springmeyer @GretaCB @allieoop @millzpaugh @flippmoke @artemp @ericfischer

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start by reading lib/publish.js and the lib/node-pre-gyp entry point, including how Run() and parseArvg() obtain setup and CWD state. Review the existing CLI flow and tests to define a refactor that permits direct mocking while preserving the CLI interface. Done requires an agreed scope and design, since this issue proposes a broad rewrite rather than a specific edit.

Written by the indexing model from the issue text.

Assessment

Tech stack
javascript, node.js
Domain
cli, tooling
Issue type
Refactor
Difficulty
5/5
Estimated time
Over a week
Activity status
Stale
Clarity
Needs clarification
Newbie friendliness
25/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.