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

Improve DX for watch plugin key conflicts #6693

Closed
tdd opened this issue Jul 14, 2018 · 3 comments · Fixed by #6697
Closed

Improve DX for watch plugin key conflicts #6693

tdd opened this issue Jul 14, 2018 · 3 comments · Fixed by #6697

Comments

@tdd
Copy link
Contributor

tdd commented Jul 14, 2018

🚀 Feature Proposal

Jest should ensure automagically that:

  • some of its built-in watch plugins key bindings cannot be overridden (e.g. q for quitting)
  • key bindings reserved by third-party plugins should not conflict (as it is likely an unintended config and should be reported).

Motivation

(For context, not this was discussed at some length already in #6473, around the end of the thread.)

Watch plugins have as an explicit design goal the ability to override built-in plugin key bindings. This lets plugins such as jest-watch-typeahead replace the built-in behavior bound to keys p and t with one that features typeahead, for instance.

However, some built-in features provided as plugins should quite likely not be overridden. For instance, the (rather essential) ability to quit the Jest watcher, using the q key, is provided by a built-in plugin, as is snapshot management, both ballpark with u and interactive with i.

Furthermore, third-party plugins should quite likely not be allowed to silently overwrite each other's key bindings, a situation that is quite difficult to debug when multiple plugins are used. Jest should check for this, report in a useful manner and exit with an appropriate code (e.g. EX_USAGE, that is 64).

Example

Here are a list of intended behaviors:

  • Registering keys p or t should remain valid.
  • Attempting to register q (quit) and w (watch usage) should error out, something like this:
Jest configuration error: watch plugin ExamplePlugin attempted to register key `q`, that is reserved for quitting Jest’s watch mode.  Please change the key configuration for this plugin.
  • I'm all for discussing whether a (all), c (clear patterns), o (only changed) and f (failures first) should be overwritable. They currently have special handling here and there in watch.js, as do q and Enter by the way. I'm also not sure what ? is supposed to do: it is handled in the code but breaks immediately, resulting in a no-op.
  • Attempting to register any key another third-party plugin already claimed (we work in positional order of the watchPlugins configuration entry) should error out, listing the conflicting plugins and the key, something along these lines:
Jest configuration error: watch plugins ExamplePluginA and ExamplePluginB both attempted to register key `k`.  Please change the key configuration for one of the conflicting plugins to avoid overlap.

Pitch

This belongs in Jest because the only code location that “sees all the keys” is Jest's watch.js module. Even if we left it up to individual plugin authors to deal with this (which is terrible DX IMHO), it would still mean Jest passes a keymap of sorts to them at construction time, so Jest's codebase is impacted.

PR

I'm starting a PR to deal with this, but would very much welcome spec refinement. In particular, I'd invite @thymikee, @rogeliog and @SimenB to continue the discussion here that we had started in #6473.

@tdd
Copy link
Contributor Author

tdd commented Jul 14, 2018

Working on a PR for this, I realized some of the tests for #6473 actually passed by mistake, as the syntax for the it.each TTS wasn't right (no dynamic parts for scenario variables). I fixed this, but would you rather see it in a separate bugfix PR, or among the PR for this key-conflicts issue?

tdd added a commit to deliciousinsights/jest that referenced this issue Jul 15, 2018
@tdd
Copy link
Contributor Author

tdd commented Jul 15, 2018

I went with a separate PR (#6696) for the test-fixing part, seemed like the cleaner thing to do. Working on a PR for the tentative spec in this issue now.

tdd added a commit to deliciousinsights/jest that referenced this issue Jul 15, 2018
- Some built-in keys remain overridable (specificically `t`
  and `p`).
- Any key registered by a third-party watch plugin cannot be
  overridden by one listed later in the plugins list config.

Fixes jestjs#6693.
Refs jestjs#6473.
tdd added a commit to deliciousinsights/jest that referenced this issue Jul 15, 2018
- Some built-in keys remain overridable (specificically `t`
  and `p`).
- Any key registered by a third-party watch plugin cannot be
  overridden by one listed later in the plugins list config.

Fixes jestjs#6693.
Refs jestjs#6473.
tdd added a commit to deliciousinsights/jest that referenced this issue Jul 17, 2018
- Some built-in keys remain overridable (specificically `t`
  and `p`).
- Any key registered by a third-party watch plugin cannot be
  overridden by one listed later in the plugins list config.

Fixes jestjs#6693.
Refs jestjs#6473.
tdd added a commit to deliciousinsights/jest that referenced this issue Jul 18, 2018
- Some built-in keys remain overridable (specificically `t`
  and `p`).
- Any key registered by a third-party watch plugin cannot be
  overridden by one listed later in the plugins list config.

Fixes jestjs#6693.
Refs jestjs#6473.
tdd added a commit to deliciousinsights/jest that referenced this issue Jul 18, 2018
- Some built-in keys remain overridable (specificically `t`
  and `p`).
- Any key registered by a third-party watch plugin cannot be
  overridden by one listed later in the plugins list config.

Fixes jestjs#6693.
Refs jestjs#6473.
tdd added a commit to deliciousinsights/jest that referenced this issue Jul 22, 2018
- Some built-in keys remain overridable (specificically `t`
  and `p`).
- Any key registered by a third-party watch plugin cannot be
  overridden by one listed later in the plugins list config.

Fixes jestjs#6693.
Refs jestjs#6473.
tdd added a commit to deliciousinsights/jest that referenced this issue Jul 26, 2018
- Some built-in keys remain overridable (specificically `t`
  and `p`).
- Any key registered by a third-party watch plugin cannot be
  overridden by one listed later in the plugins list config.

Fixes jestjs#6693.
Refs jestjs#6473.
thymikee pushed a commit to thymikee/jest that referenced this issue Aug 8, 2018
- Some built-in keys remain overridable (specificically `t`
  and `p`).
- Any key registered by a third-party watch plugin cannot be
  overridden by one listed later in the plugins list config.

Fixes jestjs#6693.
Refs jestjs#6473.
thymikee pushed a commit that referenced this issue Aug 9, 2018
## Summary

Watch plugins now are checked for key conflicts…

- Some built-in keys remain overridable (specificically `t` and `p`).
- Any key registered by a third-party watch plugin cannot be overridden by one listed later in the plugins list config.

Fixes #6693.
Refs #6473.

## Test plan

Additional tests are provided that check every conflict / overwritable scenario discussed.

## Request for feedback / “spec” evolution

The “spec” is an ongoing discussion in #6693 — in particular, the overwritability of some built-in keys, such as `a`, `f` and `o`, may be subject to discussion. This PR tracks the decisions in there and may evolve a bit still.

Ping @SimenB @thymikee @rogeliog for review and discussion.
@github-actions
Copy link

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 12, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant