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

Auto-shimming for project toolchains #23

Closed
wants to merge 2 commits into from
Closed
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
69 changes: 69 additions & 0 deletions text/0000-project-toolchain-shims.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,69 @@
- Feature Name: project_toolchain_shims
- Start Date: 2018-09-19
- RFC PR: (leave this empty)
- Notion Issue: (leave this empty)

# Summary
[summary]: #summary

Automatic management of project toolchain shims will be handled by the `yarn` and `npm` shims, which will scan the `package.json` files of the project and its immediate dependencies to determine which shims to create.

# Motivation
[motivation]: #motivation

Automatic management of shims is a core competency of Notion, in that its abstraction of the Node ecosystem workflow is incomplete without it. Automatic management of user toolchain shims is provided by the `notion install` command, but no process for automatically managing project toolchain shims has yet been described.

# Pedagogy
[pedagogy]: #pedagogy

If this feature works as intended, there is little to teach. The only significant divergence from typical JS workflows is that `./node_modules/.bin/` can be omitted when calling tools in the project toolchain:

```
$ ./node_modules/.bin/mocha # The non-Notion way
$ mocha # The Notion way
```

# Details
[details]: #details

Just prior to a successful completion inside a project, the `yarn` and `npm` shims will "scan" for dependency binaries and attempt to create shims for them. A "successful" completion, in this sense, means that the underlying tool was successfully run; it does *not* require that the tool returned a status of 0. This covers all potential scenarios in which either of those tools create new dependency binaries.

This scan will be a two-level process. First, the project's `package.json` will be inspected for all dependencies and devDependencies. Second, each of those dependencies' `package.json` files will be inspected for binaries to shim. For each binary, Notion will attempt to create a shim symlink, catching `EEXISTS` and considering it a success. Any other error will result in an appropriate message to `stderr`, but not halt the scanning/shimming process.

This same scan will also occur when pinning a version of Node using `notion use`, to ensure that shims are created for packages installed prior to setting up a toolchain for the project. Finally, the scan will be available via a new `notion autoshim .` command.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd cut the autoshim command for now. I think we should see how the tool feels with shim management kept entirely under the hood. We can always consider adding shim management commands back in after we've gotten some experience using Notion for a while.

It's not impossible to create or delete a shim by hand without a subcommand, it's just manual and icky enough to dissuade users from doing it casually. And you can always trigger autoshimming by running yarn or npm i.

But I'm now coming to the opinion that we should start by eliminating the notion shim command completely, and drop the autoshim command from this RFC.

What do you think?

Copy link
Contributor Author

@benblank benblank Oct 15, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that notion shim isn't a user-facing command, but (at least for now) I think there is some value in it for Notion developers. For example, I found myself deleting third-party shims quite frequently as part of implementing auto-shimming.

Between that and the fact that notion shim auto is already implemented and working, I'd prefer to punt this for now. It's easy enough to remove the command later, and in the mean time, we may find value in it for scripting or some such. (I liken notion shim to git's "plumbing" commands, to a degree.)

Copy link
Contributor

@dherman dherman Oct 17, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@benblank I don't want to start shipping things to users just because they're already implemented -- the point of RFCs is to try to get to consensus before we ship things. And we need to be careful about shipping user-visible features with the expectation of removing them -- removing features is very costly once you have users, and we're getting close to launching and encouraging adoption.

At the same time, I hear what you're saying about the usefulness of internal features for developers.

Let's use Cargo's feature flags functionality to put features that are only for developers behind a flag. And let's put the entire notion shim command behind a feature flag; I'm not convinced it's an appropriate feature for end users. If we decide later that it is, it'll already be in the codebase and easy to enable.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BTW your point about "plumbing" is good too. Probably the best way to think about plumbing commands is that they're for external automation (people writing extensions or scripts).

We definitely should expose plumbing commands but they're lower-level and hard to change, so we can be a little more cautious about exposing them. Starting with a feature flag seems like a good first step, and then we should think about the UI of plumbing commands. (For example, maybe we want to mark them explicitly as such in the UI, e.g. notion plumbing shim ...? But we don't need to decide that here for this RFC.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Putting notion shim behind a feature flag sounds like a great idea. I'll update the implementation PR as soon as I figure out how to do that. ;-)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.


Once the scan has completed, Notion will exit with a successful status, even if shims were unable to be created, as per [RFC 0020](0020-exit-codes.md).

Due to potential future changes (see [Critique](#critique) below), it is important that scanning be implemented in a modular fashion, such that a full scan, single dependency scan, or single shim creation can be kicked off independently.

# Critique
[critique]: #critique

## "Full scan" vs. "smart scan"

This RFC proposes that all shims be checked every time `yarn` or `npm` is run, but alternatives exist. In particular, the amount of scanning being done could be significantly reduced by parsing the Yarn/npm command line arguments and only checking the shims actually being installed during `yarn add <module(s)>` or `npm install <module(s)>`.

However, it will always be necessary to implement a "full scan" scenario for the initial install of modules — i.e. `yarn install` and `npm install`. And while a "smart scan" would increase speed, it is also potentially brittle in multiple ways. It would require Notion to both effectively duplicate those tools' command line parsers and maintain an understanding of their command structures. It would also fail to fill "holes" in Notion's shim coverage — once a shim has been missed, by any means, it will not be created until a full scan is done.

## Sync vs. async

Another alternative would be to run the scanning and shim creation process in the background, allowing an earlier return from the `yarn` and `npm` shims. Asynchrony here would make Notion feel more responsive, but introduce a race condition where the shims are not available immediately after the command completes. The two deciding factors in favor of synchronous shim creation are:

1. If shim creation is fast enough to complete between when the `yarn`/`npm` shim completes and when the user immediately thereafter tries to use one of the created shims, it *should* also be fast enough to run synchronously without affecting the "speed feel".
2. A race condition introduces the possibility for command chaining to fail:

```
$ yarn install && mocha
-bash: mocha: command not found
$ mocha
example
✓ runs a test


1 passing (11ms)
```

# Unresolved questions
[unresolved]: #unresolved-questions

- When, if ever, should shims be deleted? The ability of a user to simply delete an entire project from disk makes tracking shim use problematic, at best. What, exactly, is the behavior of a shim when the associated tool is not part of the toolchain? If it simply "forwards" to the shell, this is less of an issue.
Copy link
Contributor

@dherman dherman Oct 12, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My first instinct is to say this isn't a problem, since shims are more or less transparent. Yes, typing which foo will show a different answer than if Notion weren't installed, but otherwise the shim is supposed to behave identically to the underlying tool it delegates to (and notion which foo is a way to "see through" that difference as well), and be fast at doing its delegation. So generally I think it should be fine to leave shims in place.

The counterargument would be that shims are the most intrusive part of the Notion design, in that they impact the way developers think about how tools work on their machine.

But the problem with that is, if someone wants to delete a shim and projects keep automatically re-creating the shim, they're going to be really frustrated.

So maybe a coarser-grained approach is the right answer: if Notion is operational, you are letting it control the set of shims. If you want to get the shims out of your way, you should deactivate Notion all at once, with notion deactivate.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it simply "forwards" to the shell, this is less of an issue.

Yes, the behavior should be forwarding to the shell. So the shims are effectively fully transparent except for the behavior of which and the process tree (for example when inspecting the process table).