Small polish to the plugins API #5572

rogeliog · 2018-02-15T00:57:55Z

Summary

This is one of the items of #5478

This is the proposed API, feedback is MORE than welcome

interface WatchPlugin {
  +apply?: (hooks: JestHookSubscriber) => void;
  +getUsageData?: (globalConfig: GlobalConfig) => ?UsageData;
  +onKey?: (value: string) => void;
  +runInteractive?: (
    globalConfig: GlobalConfig,
    updateConfigAndRun: Function,
  ) => Promise<void | boolean>;
}

@rickhanlonii I changed showPrompt -> run -> runInteractive just because I felt it was a bit more explicit, thoughts?
Right now the internal plugins use the BaseWatchPlugin, I find this a bit awkward. It would be nice to get rid of it. We currently inject stdin and stdout in the constructor(this is for handling stuff like --useStderr). What would be another good way of injecting this?

Test plan

codecov-io · 2018-02-15T01:11:53Z

Codecov Report

Merging #5572 into master will decrease coverage by 0.01%.
The diff coverage is 83.33%.

@@            Coverage Diff             @@
##           master    #5572      +/-   ##
==========================================
- Coverage   61.72%   61.71%   -0.02%     
==========================================
  Files         213      213              
  Lines        7169     7169              
  Branches        4        3       -1     
==========================================
- Hits         4425     4424       -1     
- Misses       2743     2744       +1     
  Partials        1        1

Impacted Files	Coverage Δ
packages/jest-cli/src/plugins/test_path_pattern.js	`100% <ø> (ø)`	⬆️
packages/jest-cli/src/plugins/quit.js	`20% <ø> (ø)`	⬆️
packages/jest-cli/src/plugins/test_name_pattern.js	`100% <ø> (ø)`	⬆️
packages/jest-cli/src/base_watch_plugin.js	`50% <0%> (ø)`
...st-cli/src/plugins/update_snapshots_interactive.js	`44.44% <100%> (+6.94%)`	⬆️
packages/jest-cli/src/plugins/update_snapshots.js	`100% <100%> (ø)`	⬆️
packages/jest-cli/src/watch.js	`72.72% <82.6%> (+0.26%)`	⬆️
packages/jest-haste-map/src/index.js	`96.19% <0%> (-0.12%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c35b78b...98e1b49. Read the comment docs.

rickhanlonii · 2018-02-16T02:39:54Z

Awesome stuff @rogeliog

feedback is MORE than welcome

I'm on board with this API and wanted to leave a few notes to consider:

I like that runInteractive is more explicit, but it may be redundant since the full namespace is WatchPlugin.runInteractive or in another way InteractivePlugin.runInteractive. I would personally read InteractivePlugin.run as just as explicit without implying that there could be a different kind of run

Also, it may just be me but WatchPlugin.getUsageData reads like some sort of stats request for the number of times the plugin was used. Something like getDisplay, getUsage, or getUsageInfo seems more clear (with my preference being something like getKeyPrompt since that's the data returned)

Right now the internal plugins use the BaseWatchPlugin...

Agree this is awkward -- the internal reporters work the same way by extending BaseReporter. Would be good to clean both up

rickhanlonii · 2018-02-16T02:42:40Z

packages/jest-cli/src/watch.js

@@ -51,13 +51,13 @@ const getSortedUsageRows = (
 ) => {
  const internalPlugins = watchPlugins
    .slice(0, INTERNAL_PLUGINS.length)
-    .map(p => p.getUsageRow(globalConfig))
-    .filter(usage => !usage.hide);
+    .map(p => p.getUsageData && p.getUsageData(globalConfig))


Like the added check here

rickhanlonii · 2018-02-16T02:42:43Z

packages/jest-cli/src/plugins/update_snapshots.js

    globalConfig: GlobalConfig,
    updateConfigAndRun: Function,
  ): Promise<boolean> {
    updateConfigAndRun({updateSnapshot: 'all'});
    return Promise.resolve(false);
  }

-  registerHooks(hooks: JestHookSubscriber) {
+  apply(hooks: JestHookSubscriber) {
+    this._hasSnapshotFailure = true;


Is this necessary/intentional or a leftover from debugging?

rickhanlonii · 2018-02-16T02:45:28Z

packages/jest-cli/src/watch.js

-      const usageRow = plugin.getUsageRow(globalConfig) || {};
-
-      return usageRow.key === parseInt(key, 16);
+      const UsageData =


nit - should this be camelCase?

rickhanlonii

Looks great

cpojer · 2018-02-20T23:43:27Z

Awesome!

github-actions · 2021-05-12T20:06:50Z

This pull request 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.

rogeliog added 4 commits February 14, 2018 15:32

Small update to types and rename it to BaseWatchPlugin

e6667f5

Change getUsageRow to return ?UsageData and rename to getUsageData

b3c406d

A lot of renaming

beb1e1b

Change type to interface

cbb4dbc

rogeliog requested review from SimenB, cpojer, rickhanlonii and thymikee February 15, 2018 00:58

facebook-github-bot added the CLA Signed ✔️ label Feb 15, 2018

rickhanlonii reviewed Feb 16, 2018

View reviewed changes

rogeliog added 2 commits February 20, 2018 13:49

Address feedback

c0651e1

Fix eslint

98e1b49

rickhanlonii approved these changes Feb 20, 2018

View reviewed changes

cpojer merged commit 79533a9 into jestjs:master Feb 20, 2018

rogeliog mentioned this pull request Feb 21, 2018

Remove debug snapshot #5627

Merged

github-actions bot locked as resolved and limited conversation to collaborators May 12, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Small polish to the plugins API #5572

Small polish to the plugins API #5572

rogeliog commented Feb 15, 2018

codecov-io commented Feb 15, 2018 •

edited

Loading

rickhanlonii commented Feb 16, 2018 •

edited

Loading

rickhanlonii Feb 16, 2018

rickhanlonii Feb 16, 2018

rogeliog Feb 20, 2018

rickhanlonii Feb 16, 2018

rickhanlonii left a comment

cpojer commented Feb 20, 2018

github-actions bot commented May 12, 2021

Small polish to the plugins API #5572

Small polish to the plugins API #5572

Conversation

rogeliog commented Feb 15, 2018

Summary

Test plan

codecov-io commented Feb 15, 2018 • edited Loading

Codecov Report

rickhanlonii commented Feb 16, 2018 • edited Loading

rickhanlonii Feb 16, 2018

Choose a reason for hiding this comment

rickhanlonii Feb 16, 2018

Choose a reason for hiding this comment

rogeliog Feb 20, 2018

Choose a reason for hiding this comment

rickhanlonii Feb 16, 2018

Choose a reason for hiding this comment

rickhanlonii left a comment

Choose a reason for hiding this comment

cpojer commented Feb 20, 2018

github-actions bot commented May 12, 2021

codecov-io commented Feb 15, 2018 •

edited

Loading

rickhanlonii commented Feb 16, 2018 •

edited

Loading