Auto-shimming for project toolchains

[benblank]

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.

Rendered

[dherman]

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.

[dherman]

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).

[dherman]

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?

[benblank]

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.)

[dherman]

@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.

[dherman]

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.)

[benblank]

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. ;-)

[dherman]

@benblank https://doc.rust-lang.org/cargo/reference/manifest.html#the-features-section is a good place to start.

[dherman]

Coming back to this now, a couple thoughts:

I think I'd like to either remove the autoshim command from this RFC or mention that it's an experimental dev feature behind a feature flag for now.

I'm having doubts about the idea that autoshimming should fail silently. The way I'm thinking about it is, if there's a best-effort operation that happens as part of a shim, it should fail silently (or perhaps to a log file, but without affecting the console behavior); but if there's a core operation that a shim does, it should fail loudly. IOW, if autoshimming fails it's a sign that something is really wrong. I think we want this case to override the successful result of the underlying tool and print an error and return an error code.

@benblank Thoughts?

[benblank]

I can update re: autoshim once I'm back in the office tomorrow.

I'm still on the fence about exit codes; I really don't like the idea of obscuring the output of a shimmed tool, but I do see that it may make sense here. I'm happy to go with consensus on that one.

[charlespierce]

I agree with @dherman that if autoshimming is a core feature of Notion, we should show an error if it fails, so that the user knows that their system is not actually in a correct state.

@benblank To your concern about obscuring the output of npm or yarn, we can include language in the error that makes it clear what is happening (we could even include the exit code for the main tool if we want to surface that). Something like:

yarn completed successfully (exit code: 0), but there was an error setting up the Notion environment. Some commands may not work properly. Try running ... to fix this

That way we are still surfacing the result of the shimmed tool, but letting the user know that something else went wrong (and ideally giving them a push in the right direction of how to fix it).

[dherman]

Oh, this raises an interesting question: what do we do if the underlying tool fails? If it fails, I think I do share @benblank's concern about overriding the underlying tool's exit code.

So there are a few possibilities here:

1. Don't even try to autoshim if the underlying tool fails, always return the underlying tool's exit code.
2. Don't even try to autoshim if the underlying tool fails, override the underlying tool's exit code.
3. Try to autoshim no matter what, always return the underlying tool's exit code.
4. Try to autoshim no matter what, override the underlying tool's exit code.

The current RFC text is (1), and my concern is that it's swallowing a serious error -- for example, think about what happens when people do things like node -e 'blah blah blah' && gulp blah blah blah and the node shim fails to create the gulp executable.

I think (4) is terrifying in the way @benblank is worried: we shouldn't swallow failures in an underlying tool.

I'm a little queasy about (3) as well: at least it doesn't swallow the underlying tool's failure, choosing between two failures to report seems fundamentally risky.

So, what about (2)? This way, the only thing we're ever overriding is a success, in which case I think @charlespierce's suggestion is good: the error message can provide a bit of disambiguation about what exactly failed and what didn't.

[charlespierce]

volta-cli/volta#235 Raises some concerns about the overall idea of Autoshimming, and I think there's some good discussion to be had about whether it is necessary.

The RFC states "Automatic management of shims is a core competency of Notion", but after seeing the concerns raised in the above issue, I'm not so sure that's the case. In my experience, it's not commonly expected that locally installed packages will be available within the shell. Rather, those are typically invoked using the scripts entry in package.json, with npm run or yarn run.

The only tools that we really want to shim are tools that are normally installed globally (e.g. ember-cli or typescript), since those are invoked directly from the shell. In that case, I think it's reasonable to require the user to opt-in to having those managed by notion in some way.

I can see a couple different ways to support that:

1. Support it through the ongoing work to Install Package Binaries, where the user does notion install ember-cli and we install a global version that they can use everywhere. Then if they invoke the shim from within a project, we can check if the project has a local version and delegate to that instead of the global version they installed on their user toolchain. This has the benefit of fitting into the common workflow, where you globally install something and then use it everywhere, but the downside of requiring users to globally install something even if they only want to use it in a shim situation within projects.
2. Document and support the notion shim command, which would allow users to create shims for any executables they want, which delegate to the local project if available and fall back to the normal PATH if there isn't anything there. This has the benefit of letting users install shims for local executables without having to globally install something, but is a little trickier to explain and exposes some of the internals of Notion.

Of those two, I feel like the first fits better into the common OSS workflow, and is easy enough to explain (You globally install tsc just like you normally would, but if there is a local version specified in a project, we will use that instead of the global one for consistency).

[dherman]

In my experience, it's not commonly expected that locally installed packages will be available within the shell.

I think this is really a key point that I'm embarrassed to have overlooked for so long.

To me, this is pretty dispositive that while auto-shimming was a good area of exploration (and thank you for your work on it, @benblank!), having a single, user-managed toolchain is a clearer, safer, and more understandable model.

Again, even if we decide against auto-shimming, this RFC was awesome work and super helpful in driving out discussion. I really appreciate it!

[stefanpenner]

@charlespierce good write up! I can get behind that.

[dherman]

Closing as we've decided to move towards a more user-controlled model. Even though we're closing this RFC, it was great work and really helped us think through the details!