Update roadmap

[demurgos]

Hi,
Last year I did some work on spawn-wrap on my fork to support hooking the Node inspector into Node processes deep in the process tree. I eventually built a library dedicated to this use case, but my spawn-wrap fork still had a lot of work put into it to fix some issues and provide new features. I would like to merge the changes so others can benefit from them. I started sending small PRs, this issue is intended to give the outline of the changes I made and discuss them in a larger scope.

There were many small changes that together improved the reliability of the lib and fixed various Windows issues: using safe path manipulation functions, escaping the values more explicitly, reducing the number of extra files (no settings.json), avoiding mutations of the user-supplied options object, etc. One of the intended goals is to reduce the amount of patches to Node internals. It does not remove all internal patches (see #89).

There were also some larger changes:

• Provide an API that does not patches the internals. This allows to spawn concurrent sub-processes with different options without interference. This is achieved by creating a context (options + shims) and then using it to create a spawn function. This function rewrites its arguments to inject the wrapping logic and forwards the arguments to the native spawn function. The general idea is the following:

  // lib code
  function wrapSpawn(ctx) {
    return function(...args) {
      const wrappedArgs = munge(ctx, args);
      return cp.spawn(...newArgs);
    }
  }
  
  // user code
  const ctx = new SwContext(options);
  const wrappedFn = wrapSpawn(ctx);
  const child = wrappedFn("node", [...]);
  
  
  const ctx2 = new SwContext(options2);
  const wrappedFn2 = wrapSpawn(ctx2);
  const child2 = wrappedFn2("node", [...]);

• Add static type checks with Typescript. The library is stable now so it would benefit from being annotated and checked. Using types helped me a lot to understand how spawn-wrap works: I coud annotate what were the expected values and what each function returned. As a bonus it enables better autocompletion and documentation for the consumers. I wrote a small post about it: it's definitely worth it for a stable lib.

• The only breaking change I eventually introduced was a simplification of how the wrapper code is executed. I looked into all the dependent packages and how they use spawn-wrap. They fall into two categories: they either want to ensure some VM options are enabled or run some code before/after the user code. For the second case, the wrapper code often receives some config from the root process but the current design makes this use case hard (it involves pushing and poping args or env vars). Starting the user-code from the wrapper uses a runMain function exported by spawn-wrap, but this function does not work outside of the wrapper. I eventually settled on a design where the wrapper may export a function, the shim then calls it with a config object containing data from the parent process and a method to start the user code. See the migration section on my fork

• Finally, the largest of all changes was adding support for dynamic arguments (Dynamically create child_process args. #47). This allows a very powerful abstraction were the root process can watch process creations at any depth in the process tree, modify their arguments, directly read their stdin/stdout/stderr, etc. This is built on top of the current static version using some IPC. Ultimately, I don't think it should be merged here: it is great but too complex. It could be published as a separate package depending on spawn-wrap for users needing this kind of control.

I opened a PR (#77) with all those changes, but it should not be merged: it's just to show you the code I ended up with. Instead, I am extracting the changes one by one and sending small PRs. This allows to discuss each change individually and catch errors (thanks for the reviews).

My first step was to update the requirements and remove legacy code. This is almost done. I'll send a PR soon to split the library into smaller modules so it's better to see the structure of the lib at a glance. At this point, the library should still be functionally the same: it would be a good point to do a release and check that the refactoring did not break anything. The main change would be the increased minimum Node version.

Once this is done, I'd like to merge new features: encapsulations with contexts and static type checks. This is also the step were I added most of the minor fixes.

Then, depending on the discussions, I'd propose the new API were the wrapper module exports a function called by the shim.

/cc @bcoe @JaKXz @coreyfarrell

[coreyfarrell]

Re TypeScript I'd like to ask that we avoid requiring a build step (so .d.ts files instead of converting actual sources to TypeScript). Writing code that requires a build step makes it impossible to install from git branches - so this creates a burden when trying to test patches with other packages.

[demurgos]

Okay, that's a good point. I remember reading that npm planned to add/already had support to build packages from Git branches but I don't know how it works. I'll keep .d.ts files if I don't figure a solution.

That's actually the reason why I published releases of my fork when I wanted to use it in other projects (instead of pointing to a git hash) so I understand the pain.

Edit: npm runs prepare scripts when installing git branches. I don't know if it installs the devDependecies for these packages or not.

Edit2: Using npm inside a vanilla JS package, the following command adds the dependency and builds it transparently: npm install "demurgos/spawn-wrap#b44056011f946e698f04d099d96e82d0a66f8a52". It's very impressive, if I didn't know the project uses Typescript I wouldn't have noticed. The downside is that yarn (and probably other package managers) does not support this. I think that this is good enough to test changes with other packages, and if you really need to distribute it to consumers then you can publish it (I consider git dependencies to be a development feature that should not be used by packages published to npm).

[coreyfarrell]

To be honest another issue for me personally is that I'm not a TypeScript developer. I'm OK with .d.ts, but as soon as a library converts the actual source to TS I'm out.

Edit: For the record I'm only one maintainer here, others may feel differently than I do. Just being honest about my criteria for me participating in maintenance of a module.

[demurgos]

#91 (split into small modules) is closed to being merged.

It would be good to publish a new version following it. @bcoe proposed to use a pre-release (next tag): I think it's a good idea. We can continue merging PRs and publishing on next and wait for feedback before publishing on the latest tag.

[coreyfarrell]

I'd definitely like to see the path.join used over string concatenation first but also I'm wondering what other breaking changes you have planned. Keep in mind master has already dropped node.js 6 so the next release will be semver-major. Granted we could do 2.0.0-alpha.1 or something like that and still do more breaking changes. That said nyc still has outstanding reviews and I'm still working on a breaking change to the reporting API, so to be honest I'm not really ready to do anything with a new spawn-wrap release yet.

[demurgos]

Agreed, string concatenation can be part of the initial release.

I thought that we could release multiple spawn-wrap@next releases until we are satisfied.

I planned two breaking changes:

• Requiring Node 8
• Update how wrappers are defined

They don't have to be part of the same major version.

A possibility would be for spawn-wrap@2.0.0 to only update the Node requirements. Extra patches and features can be added to the 2.x major release. It may also be possible to provide the new wrapper API as an opt-in and support both ways in parallel. And then the spawn-wrap@3.0.0 version would change the default wrapper API when we're sure that all the cases are covered.

The wrapper update would be "nice to have" but isn't a high priority compared to the fixes.

[coreyfarrell]

I'd definitely like to see the path and string concatenation handled first (path.join / path.resolve for paths and JS template literals for strings). Once that's done I'm likely to do a fresh review of the whole repo. Many of your PR's I did not review as new code but as modified code knowing there would be follow-ups, so I'm likely to do a follow-up PR to deal with that sort of thing (I'll CC you in the PR).

One thing about this release, how were you planning to test these releases? Currently you can create a package.json containing:

{
  "devDependencies": {
    "nyc": "^14.1.1",
    "spawn-wrap": "istanbuljs/spawn-wrap#master"
  }
}

This would allow you to test the master version of spawn-wrap with the current release of nyc. Once we bump the spawn-wrap version in master this will no longer be possible as npm will see the git version of spawn-wrap as incompatible with the version nyc depends on.

[demurgos]

You make a good point.

I thought that you were planning to publish next branch of nyc too, but the workflow you describe here is simpler/requires less coordination. Maybe we should simply not publish anything to npm and tell people to use the code you posted?

[coreyfarrell]

well at a certain point spawn-wrap master will be tagged 2.0.0 at which point nyc@14 will stop using it. Pointing to spawn-wrap#master should only be done for testing. For any production use where spawn-wrap has fixes people need they would need to use "spawn-wrap": "istanbuljs/spawn-wrap#ebcee7e" or another specific commit SHA from before we do an actual 2.0.0 release.

I do plan to publish a next branch of nyc but we're not there yet, have some reporting work I'm still trying to iron out.

[coreyfarrell]

@demurgos any chance you could make the path.join changes requested in one of the reviews? nyc is getting closer to being ready for a pre-release, I'd like to incorporate the updated spawn-wrap in this. I know that the path.join change isn't breaking but I'd still rather get it into 2.0.0.

[demurgos]

@coreyfarrell I'll send the PRs tomorrow, with path.join and all the fixes.

[coreyfarrell]

2.0.0-beta.0 is now published to the npm next tag.