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

Todo items for watch plugins #5478

Closed
6 of 7 tasks
rogeliog opened this issue Feb 6, 2018 · 10 comments
Closed
6 of 7 tasks

Todo items for watch plugins #5478

rogeliog opened this issue Feb 6, 2018 · 10 comments
Milestone

Comments

@rogeliog
Copy link
Contributor

rogeliog commented Feb 6, 2018

#5399 was merged, and it rewrites some parts of the watch mode to support third party plugins.

There are still some follow up items that need to be done.

@thymikee
Copy link
Collaborator

thymikee commented Feb 6, 2018

Find a better name for getUsageRow(), potentialy getUsageInfo()

How about getDisplayUsage() or even displayUsage()?

@rogeliog
Copy link
Contributor Author

There is a problem that I've been encountering while implementing Jest plugins to stress the API.

In an ideal world, I should be able to do this:

Implement the pattern mode by simply using inquirer.

a

Here is the code for it.

const inquirer = require('inquirer');
const ansiEscapes = require('ansi-escapes');

class PatternPlugin {
  constructor() {
    this._pattern = '';
  }

  apply(jestHooks) {
    jestHooks.shouldRunTestSuite(async testPath => testPath.includes(this._pattern));
  }

  run() {
    console.log(ansiEscapes.clearScreen);

    if (this._pattern.length) {
      console.log('Active filter:', this._pattern, '\n');
    }

    return inquirer
      .prompt([
        {
          type: 'input',
          name: 'pattern',
          message: 'Type pattern:',
        },
      ])
      .then(({ pattern }) => {
        this._pattern = pattern;
        return true;
      });
  }

  getUsageInfo() {
    return {
      key: 'e'.codePointAt(0),
      prompt: 'to enter mega filter mode',
    };
  }
}

And we could get fancier by using more inquirer features and really easily create this:
a

So what is the problem with this?

Jest sets stdin.setEncoding('hex'), which means that tools like inquirer don't work out of the box.
Here is my current workaround to get it to work, but it seems awkward.

  run() {
    process.stdin.setEncoding('utf8');
    console.log(ansiEscapes.clearScreen);

    if (this._pattern.length) {
      console.log('Active filter:', this._pattern, '\n');
    }

    return inquirer
      .prompt([
        {
          type: 'input',
          name: 'pattern',
          message: 'Type pattern:',
        },
      ])
      .then(({ pattern }) => {
        this._pattern = pattern;
        process.stdin.setRawMode(true);
        process.stdin.resume();
        process.stdin.setEncoding('hex');
        return true;
      });
  }

I'm not sure what the correct solution would be, but it would be awesome for inquirer to work out of the box.

  • Should Jest set the encoding to utf8 and then back to hex when a plugin gets activated?

cc: @cpojer, @thymikee, @rickhanlonii, @SimenB

@SimenB
Copy link
Member

SimenB commented Feb 21, 2018

Supporting inquirer out of the box sounds really appealing. Any downsides to switching encoding? Should a plugin be able to dictate its wanted encoding? Why do we use hex in the first place?

I also thought when I looked at your gif that it would be cool if that fancy filter also had typeahead. Thoughts on reusing logic between different plugins? Or have them extend one another.

@cpojer
Copy link
Member

cpojer commented Feb 21, 2018

I think the encoding was just for detecting the keys. utf8 should equally work, no?

I really like this idea, btw.

@rogeliog
Copy link
Contributor Author

Got it, thanks!

@SimenB

I also thought when I looked at your gif that it would be cool if that fancy filter also had typeahead. Thoughts on reusing logic between different plugins? Or have them extend one another.

Yes, we just need to come up with a nice interface for injecting some of the SearchSource stuff https://github.com/facebook/jest/blob/master/packages/jest-cli/src/search_source.js#L139, I was thinking of two options.

  1. Inject only the file paths in a jestHooks.onFilePathsChanged(filePaths: Array<string>) and leave the rest to the plugin.
  2. Find a way to access some of the searchSource queries, like findRelatedTests.

I think we should start with the first one, given that it seems less opinionated, lower level and simpler to implement.

We could probably call it from here https://github.com/facebook/jest/blob/master/packages/jest-cli/src/watch.js#L155-L180, Although I'm not sure if we should pass all the hasteFS files, or just testFiles or what would be a good interface for it.

@SimenB
Copy link
Member

SimenB commented May 24, 2018

Checked off the encoding point 🎉

@SimenB
Copy link
Member

SimenB commented Sep 20, 2018

Aaaaand the config one. Just CI env left. Not sure about that one, though

@SimenB SimenB added this to the Jest 24 milestone Oct 16, 2018
@SimenB
Copy link
Member

SimenB commented Oct 16, 2018

@rogeliog thoughts on opening up a separate issue for the CI stuff?

@rogeliog
Copy link
Contributor Author

Yes, let's open a separate issue... Although I think we can just close it and then open a separate issue if it is ever needed.

@SimenB
Copy link
Member

SimenB commented Oct 17, 2018

Yeah, let's do that instead 🙂

@SimenB SimenB closed this as completed Oct 17, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants