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

[4.0] setup wizard #590

Closed
connectdotz opened this issue May 26, 2020 · 13 comments · Fixed by #655
Closed

[4.0] setup wizard #590

connectdotz opened this issue May 26, 2020 · 13 comments · Fixed by #655

Comments

@connectdotz
Copy link
Collaborator

connectdotz commented May 26, 2020

This is to discuss the 4.0 (#576) setup wizard ideas:

Motivation

Traditionally vscode-jest aimed to provide the magical "working out of the box" user experience by auto-detecting the user's environment types then locate and execute jest with a "default settings" accordingly. While this works well for some basic/simple development environments, it often falls short for the real-world, more sophisticated ones. The solution has always been to encourage users to customize the extension with the provided settings. But since most new users are not familiar with these settings and with the expectation that the extension should just work out of the box, it has become one of the biggest sources of confusion and frustration among our users.

To address this problem, we propose to replace the "auto-detect/auto-generate defaults" on each run with a user-directed explicit config at the first launch. This not only ensures the extension can work exactly and correctly as the user desired but also greatly simplified the extension (simply follow the config) while expanding supports for more development env variations.

Questions we asked ourselves

  • why can't we make the auto-detection better?

While we can certainly add more logic to detect more variations, it is simply impossible to cover all frameworks and configurations a user can possibly use. It is not surprising that almost all well-developed tools/frameworks offer some settings to help users express their special requirements.

Also with more code to detect user environments, we are increasing the complexity and reducing the stability of the extension. A faulty detection code had crashed the whole extension and caused a lot of users not able to see their test results in the past. Yes, more testing might help but if we can eliminate the code altogether, wouldn't it be better?

  • Ok, maybe not necessarily covering all frameworks, but why not "X", it is very commonly used!

The problem is everybody has their own "X", how do we decide? where to draw the line?

  • Let's address the elephant in the room, wouldn't this up-front wizard bad user experience comparing with "just work"?

If "just work" works every time then yes. But the reality is sooner than later one will need to set these configs anyway. Often after hours of researching why the extension is not working, followed by more research on how to configure the extension.

I believe if we design the wizard right, the one-time setup experience should be not only acceptable but the guided experience will actually be much better than doing it on your own.

Overview

The goal of this task is to move away from the current "default" model (if config not set, detect CRA otherwise run jest from node_modules) toward the "required explicit settings" model with a wizard-like helper.

The wizard will present an interface to gather the necessary info from users, such as development env (CRA or plain jest initially), present the default settings (such as jest.jestCommandline) accordingly, help users make modifications if needed, then record these settings and start the rest of the extension.

An env specific default settings can be a list of statically defined value + vscode's debug config template. Therefore adding support for a new environment could be as simple as adding a new set of such info during deployment without little or no code change.

Implementation

To implement a command (i.e. can be triggered via Command Palette as well as internally invoked by the extension at startup) that walk users through a 2-step process to set up their

  1. minimal vscode-jest config, such as jest.jestCommandline
  2. debug config,

and record the result in vscode's settings.json, launch.json, or the multi-root workspace equivalent.

when to show wizard?

  • if wizard has never been run
  • when user manually trigger the wizard via the Command Palette

wizard workflow:

  • step 1:
    • Show a selection for possible env that we currently "support", i.e. with some default settings for the given env: "plain jest", "CRA", "Other"...
      • if this is a multi-root project, we will need to repeat this for each folder
      • for "Other" env, we could provide some simple instructions and user will need to provide the minimal settings such as jest.jestCommandline.
    • show default configuration based on the user's selection above. It should at least include jest.jestCommandline. If this config already exists, it should override the default. Give short instructions and allow users to customize it if needed.
    • write the result to settings.json or equivalent (in multi-root env)
    • if the user chooses to abort the wizard, the settings will not be updated. And if jest.jestCommandline does not exist, the extension should be deactivated.
  • step 2:
    • do the same for debug-config. Based on the choices from the previous step, we can then use the existing debug config snippets/templates as the baseline. Confirm if to use the same jest command line from the previous step, update the snippet accordingly. Also be aware if there is already an existing config, confirm before override.
    • writes the result to launch.json or equivalent (in multi-root env)
    • if the user chooses to abort at this point and there is no debug config created, it should be ok to activate the extension but if users do debug we should ask them to go through the wizard step-2?

I know this shift might be quite different from the current paradigm, please don't hesitate to let us know if there is anything missing or does not make sense...

@connectdotz connectdotz mentioned this issue May 26, 2020
15 tasks
@connectdotz connectdotz added this to the v4.0 milestone May 26, 2020
@stephtr
Copy link
Member

stephtr commented Jun 13, 2020

Sounds good. Just to link the issue, the setup wizard should also offer support for Yarn pnp (#594).

@Tymek
Copy link
Contributor

Tymek commented Jul 2, 2020

please, don't show this on every activation. user should not be prompted unless something isn't working

@rossknudsen
Copy link

I've been taking a look at this and wanted some feedback. The code that attempts to figure out what the CLI command should be to run Jest is:

export function pathToJest({ pathToJest, rootPath }: PluginResourceSettings) {
  if (hasUserSetPathToJest(pathToJest)) {
    return normalize(pathToJest);
  }

  if (isBootstrappedWithCreateReactApp(rootPath)) {
    return 'npm test --';
  }

  const p = getLocalPathForExecutable(rootPath, 'jest') || 'jest' + nodeBinExtension;
  return `"${p}"`;
}

Logic:

  1. If user has configured path, use that
  2. If CRA, use 'npm test --'
  3. If can resolve Jest in node_modules folder, use that
  4. Otherwise assume global Jest

I think a better UX would be to try and cater for additional cases that should be supported out of the box, i.e. without user interaction:

  • Yarn workspaces
  • Lerna workspaces
  • Nx.dev workspaces
  • Simple apps with a script key - 'test' or 'jest'

Probably should also attempt to figure out if the source uses Yarn or NPM and adjust commands appropriately.

@connectdotz
Copy link
Collaborator Author

I think a better UX would be to try and cater for additional cases that should be supported out of the box, i.e. without user interaction

We have gone down that path before... unfortunately, it is never enough and often error-prone, not to mention the maintenance nightmare to keep up with the ever-growing frameworks and their evolutions. Therefore the decision is that instead of complicated "guessing" logic that could be wrong and frustrate users, we would help users help themselves by providing a wizard or template as described here.

Your suggestion of possible variations is good, but instead of "auto-detection," we could maybe just provide "templates" users can select then customize...

@rossknudsen
Copy link

Sure, I was just going through the requirements above. There is some implication that some things should work out of the box. Based on @Tymek's comment:

please, don't show this on every activation. user should not be prompted unless something isn't working

I would take that to mean we should make a best effort to recognize a reasonable set of configurations. Which is already the case, CRA and standard repository structures are already handled. @stephtr's request to support Yarn PnP configs out of the box expands on that set.

Now to your point, you can't cater for 'ever-growing frameworks and their evolutions'. I whole-heartedly agree and my own experience with maintaining a Jest extension is that there are some quite diverse setups out there. So I think there is a balance required. A list of maintained setups is probably recommended, based on the previous paragraph's discussion. In working on this ticket I also have a proposal on how to make this manageable. I propose we attempt any auto-recognition processing that is actively maintained and failing that, fallback to the wizard to aid the user in setting up their own configuration.

Here is the auto-configuration steps I'm proposing:

Have a common interface can identify a particular type of repository:

interface RepositoryIdentifier {
  match: () => boolean;
  getRunCommand: () => string;
  getDebugCommand: () => string;
}

You then have a bunch of these identifiers registered for each supported repository type (CRA, Yarn PnP, standard repo). Then simply find the one(s) that return true for the match function:

const identifiers = getIdentifiers(settings);
const matchingIdentifiers = identifiers.filter((x) => x.match());

Then handle the case where there is zero, one or multiple matches. The multiple matches scenario is interesting in that we could loop over each one to see if any of them result in a working configuration. Here are some other thoughts on this idea:

  • Extensible, can support more repo/framework setups in the future by adding more RepositoryIdentifiers.
  • RepositoryIdentifier should be unit testable.
  • RepositoryIdentifier.getDebugCommand could return a DebugConfiguration. That gives each repo type the flexibility to have specific logic to ensure that they work while the actual running logic should be able to remain fairly static.
  • Plugins support?

And if everything fails, then we will offer the user a wizard to set the command. BTW a user configured command will override all of the above as it currently does (although it raises the question that if that command fails, we could try the built-in ones...).

@connectdotz
Copy link
Collaborator Author

@rossknudsen, thanks for your comments. It prompted me to update the design to clarify some of the confusion you and others have mentioned. Feel free to review it again.

This might be redundant, but let me still address some specific comments:

There is some implication that some things should work out of the box.

I updated the design above and hopefully clarified this point. The idea of the wizard is to move away from "something should work out-of-the-box and customize when it doesn't" model. Instead, we want to move to "every user will explicitly config the extension once, with the help of a wizard"

Regarding @Tymek's comment:

please, don't show this on every activation.

The wizard will not show on every activation, only the first time.

user should not be prompted unless something isn't working

On the surface, it sounded reasonable, but what does "something isn't working" really mean? When jest failed? When the spawned node process failed? jest or node can fail for many reasons, not all are related to this extension... How do we know when to show the wizard? parsing error messages 😱 ? And if we guessed wrong, will users see the wizard repeatedly 😱 ? Not to mention not all "isn't working" involves errors, for example, the user might want to use yarn test but the extension used the default jest from node_modules.

Fortunately, we don't need to deal with the issues above when we stop relying on auto-detection/default-setting-generation. Please see the updated design above.

Regarding the scope of the initial release of the wizard... this is a high impact change, let's start with just the currently supported env, i.e "CRA", "plain", and "other" (see the implementation section above). We can add more environments once the wizard is battle-tested.

To be safe, we could also consider an incremental approach... maybe keeping the current auto-detect/auto-generate-pathToJest logic exactly as it is and deploy wizard only as a manual command from the command palette before turning it on for auto-launch.


@stephtr @seanpoulter @orta given this is somewhat a paradigm shift, do you guys have any concerns or issues we should address before moving on to the implementation phase?

@connectdotz
Copy link
Collaborator Author

@rossknudsen are you still interested in working on this issue?

@rossknudsen
Copy link

Sorry for the late reply. I don't have the bandwidth for this one at the moment. I think you'd need to implement this using webview which is a reasonably large undertaking.

@stephtr
Copy link
Member

stephtr commented Jan 15, 2021

Sorry for bringing this up again:
When briefly working with projects utilizing jest I again noticed how much I like this extension's automatic config. Would there have been a message for setting up the extension instead, I probably would have ignored it in most cases, since I was just briefly working on those projects. Also, when working on (larger) projects by others, one doesn't want to commit VS code extension settings to the repository, if they aren't using VS Code already.

However, what I would completely agree with is that we have to make it easier to setup this extension and to push the users to do so (slightly, but not too much).
One example would be the debug CodeLens, on which our automatic detection probably works worst. In addition to the "Debug" button, we could add a "Setup debugging" button, if no custom config is present:
image
For the Runner we could add a menu to the Status bar button, which contains the possibility to setup jest via the setup wizard, switching watching mode and opening the output.

My gut feeling would be that the automatic detection we currently have works well in 90% – 95% of all uses (it would be interesting but probably hard to get numbers about this). Combining this with the detection of the task runner not being able to start (which would result in the extension offering the user the setup wizard) would add another few percent. I think the heuristic doesn't have to be perfect, as long as we make it easy to overrule it and the fraction of wrong positives is sufficiently low; I would still prefer this situation over users not using this extension because they are too "lazy" to go through the setup wizard.

This is however just my very personal opinion on this topic, in the interest of this extension I'm also fine with completely switching to the setup wizard configuration if you think that this would be the better choice. For me there is no further reasoning necessary, I accept your standpoint and explanations 😉

@connectdotz
Copy link
Collaborator Author

connectdotz commented Jan 19, 2021

@stephtr I think you have a good point. I started to look into the wizard in the past few days and I think I would agree that the "prompt wizard if failed" you proposed might be a good hybrid approach, which we can still offer zero-config for some while helping the failed use cases with the wizard when most needed.

In addition to a setup command that users can do it any time they want, we can add a setup wizard button at the error panel where we told them jest failed too many times, so they can also start the wizard easily from there.

We already give people a warning when the debug config doesn't exist. I think we can use the same approach to offer wizard in that message panel, instead of using codeLens I think...

@bogdan-calapod
Copy link

Hey there :D

I was just looking through the issues for something unrelated and found this issue. I find the idea of a wizard awesome, as it cleans up the "magic" of the whole process.

If it isn't time-sensitive, I can take a jab at designing some mockups for the wizard if you want. I can also try my hand at implementing the UI - if I understand correctly these kind of things are done via a WebView, right ?

@connectdotz
Copy link
Collaborator Author

@bogdan-calapod thanks for trying to help, I think we are pretty close with the prototype, hopefully, the PR will be submitted this week. The UI is not that complicated, so end up using vscode's quick input and messaging API instead of webView.

We can definitely use your help though, once the wizard is submitted, would love to have you play with it and let us know if it can be improved.

@bogdan-calapod
Copy link

Sure - I'd gladly help with testing :D

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants