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]

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]

https://github.com/notion-cli/notion/issues/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!