use kebab-case

[vhdirk]

This PR fixes #923.

Depends on jest-community/jest-editor-support#91

[coveralls]

Pull Request Test Coverage Report for Build 3307452593

• 14 of 14 (100.0%) changed or added relevant lines in 2 files are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage remained the same at 96.22%

---

Totals
Change from base Build 3304122522:	0.0%
Covered Lines:	3019
Relevant Lines:	3083

---

💛 - Coveralls

[connectdotz]

hi @vhdirk Thanks for submitting this PR 👍

In addition to the files you updated, there are a few other places that require similar treatment, such as:

• package.json
• DebugConfigurationProvider and its tests
• process-listeners and its tests: (there is regex looking for watch flags, not sure if it will be impacted, please check when you are at it, thx)

to be consistent. we should probably also update these test cases

• setup-jest-debug.test.ts
• wizard-helper.test.ts

[vhdirk]

Hi @connectdotz

I've gone over all the files you mentioned and changed every occurrence I could find. I've also looked at the regexes in process-listener. Based on some manual tests I did I am pretty sure jest will not change the messages it emits based on the use of camelCase vs kebab-case flags.

[connectdotz]

@vhdirk looks good.

There are some issues in the jest-community/jest-editor-support#91, let's finish that before merging this, so we can test these changes as a whole. While the changes are quite mechanical, it is significant, we should do more testing...

[vhdirk]

@connectdotz Thanks for reviewing. For what it's worth, I've been using this as my daily driver for at least a week now. No issues to be seen :)

[connectdotz]

@vhdirk I tested a bit with the new kebab arguments, and bumped into a problem with react-scripts. It seems react-scripts only recognized "--watchAll=false" but not "--watch-all=false". I opened an issue facebook/create-react-app#12801 and hopefully, it will be addressed for future versions.

This means we might have to put the CI flag (in jest-editor-support) back (but I vaguely remember there were some other conflicts??), or switch between watchAll and watch-all selectively, which definitely sucks and not sure if we can reliably detect react/react-native... thoughts?

[connectdotz]

What do you think about this: maybe we just pass a flag (a new setting) to jest-editor-support which then converts all jest arguments accordingly. That way, we don't need to hard-code or change anything on the vscode-jest side (maybe?)...

[vhdirk]

I could also make a PR for facebook/create-react-app#12801, but I suppose it would still mean that every single app ever made with it will also be broken?
That's quite a bummer.

I think adding a flag is pretty much the only way forward, though I don't quite understand how that would mean that we don't have to change anything in vscode-jest. I specifically ran into issues there.

[connectdotz]

I could also make a PR for facebook/create-react-app#12801, but I suppose it would still mean that every single app ever made with it will also be broken?

Yes, all existing react projects will still be broken. The patch will only help future projects.

I think adding a flag is pretty much the only way forward, though I don't quite understand how that would mean that we don't have to change anything in vscode-jest.

On the top of my head, other than the DebugConfigurationProvider, all other jest options are packed in JestProcess.startRunner, which just pass these options to jest-editor-support/Runner, where I hope we can then do the conversion based on the new setting, before spawning the actual jest process.

Regarding DebugConfigurationProvider, only the older version of the debug config will need to switch by the setting, the new version (v2) already give users' the control of the options so no need to do anything there...

[vhdirk]

How do you feel about adding https://www.npmjs.com/package/change-case as a dependency? It would make the code quite a bit cleaner.

[connectdotz]

I think it might be a bit overkill, given we only need to consider camel => dash/kebab.

[vhdirk]

The same author does offer the library in smaller parts:

• https://www.npmjs.com/package/param-case
• https://www.npmjs.com/package/camel-case

If we have to support both options, I'm not very keen on littering the code with stuff like kebabCase ? '--watch-all' : '--watchAll'

[connectdotz]

It seems this is pretty much a single line function, I did a quick a test and it seems to work:

it('convert to kebab case', () => {
  const s = 'testNamePath'.replace(/(\B)([A-Z])/gm, '-$2').toLowerCase();
  expect(s).toEqual('test-name-path');
});

Therefore, I think we are ok not to add a new dependency.

Overall, I am thinking we will leave vscode-jest jest flags unchanged, but adding a new setting jest.useDashedArgs (jest's document used the term dashed args). We will pass that argument as part of the runnerWorkspace, which is constructed here. You can follow "settings.shell" to see how we pass a setting all the way through to jest-editor-support.

On the jest-editor-support side, the ProjectWorkspace and the related types need to be modified to take in the new flag. And the actual argument case conversion will only need to take place in Runner._getArgs(), if jest.useDashedArgs is true, otherwise (undefined or false), do nothing, i.e. we will not do dash => camel conversion. jest-editor-support will only do a very limited single-direction conversion for the args it added or from options.args.

I hope it makes sense, let me know what you think.

[connectdotz]

Hi @vhdirk, are you still interested in finishing this PR?

[connectdotz]

close via #1034. Thanks to everyone who participated.