remove yargs-unparser; run yargs only once #3833
Conversation
cspotcode
changed the title
|
@cspotcode did you disable the pre-commit hooks? eslint should have fixed the lint failures automatically (assuming it could) |
|
I didn't run eslint (EDIT or prettier or anything else), no. I still need to cleanup this PR before it's ready to merge; I'll fix the linter failures and squash all those commits. |
Pass parsed args object to spawned process as JSON string.
cspotcode
force-pushed
the
feature/json-instead-of-yargs-unparser
branch
from
7e072e1 to
654fee2
Compare
|
This is ready for review. I didn't need the base64 encoding on Windows, so I moved that to another branch just in case it's relevant in the future. Installed size drops by about half, with more modest improvements in startup time. |
| @@ -123,7 +121,8 @@ debug('final node args', nodeArgs); | |||
| const args = [].concat( | |||
| unparseNodeFlags(nodeArgs), | |||
| mochaPath, | |||
| unparse(mochaArgs, {alias: aliases}) | |||
| '--preParsedJsonOptions', | |||
| JSON.stringify(mochaArgs) | |||
| ); | |||
There was a problem hiding this comment.
This way duplicate alias options don't get eliminated. With unparse() we get a clean array without any aliases, so I'm not sure wether we can do without unparse() as easily.
It's a very good idea not to parse a second time in "/cli.cli.js".
There was a problem hiding this comment.
I don't think it's necessary to remove duplicate alias options using this method. They won't pass through yargs' parser again; the object is passed straight to mocha.
When mocha v6 re-parses the output of yargs-unparser, it creates those duplicate aliases all over again. So either way, the output object contains aliases, and mocha is built to handle it.
There was a problem hiding this comment.
Yes, I think the mocha instance looks quite messy with all those aliases, but it seems to be able to handle them correctly.
[...]; the object is passed straight to mocha.
This could be too early, since the coerce function of reporter-option hasn't run yet?
| return exports | ||
| .createYargs() | ||
| .parse(argv, Object.assign(loadOptions(argv), {command: {}})); | ||
| }; |
There was a problem hiding this comment.
I haven't understood yet (neither in the current master) why the object returned by loadOptions() is passed as context parameter to parse().
- the coerce functions are not called that way, see --reporter-option(s) flags in .opts files are ignored when running with _mocha #3826
- duplicate alias option don't get eliminated
There was a problem hiding this comment.
Yeah, sounds like a separate bug. EDIT: I'm also not sure why mocha would even allow aliases in a config file; seems like it introduces confusion without much benefit.
There was a problem hiding this comment.
Even node flags are allowed in config files. When run with _mocha they are useless, I don't think they get filtered out, probably they are just ignored.
|
I tried to fix that separate bug mentioned above: juergba/issue/3826. You will not like it, because I'm using "yargs-unparser". Maybe you can merge my fix into this PR, otherwise I would open a seperate PR. |
|
The proper solution is to stop using yargs for option processing that does
not apply exclusively to argv positionals. Using unparse is masking this
deeper code smell.
If yargs' API is only running coerce functions against values from argv,
then the coerce functionality is *not* intended for loading options from a
config file. We should not use it for that purpose. Instead, the coerce
functions should be moved outside of yargs and executed as a post-process
step. If there is coerce logic that applies exclusively to argv values and
not to values from a config file, then it should be kept in yargs coerce
functions.
…On Wed, Apr 17, 2019, 9:31 AM Juerg B. ***@***.***> wrote:
I tried to fix that separate bug mentioned above: juergba/issue/3826
<https://github.com/mochajs/mocha/tree/juergba/issue/3826>.
You will not like it, because I'm using "yargs-unparser". Maybe you can
merge my fix into this PR, otherwise I would open a seperate PR.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#3833 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAW-uNuEDYghd7hae2uLSBeK_bZcb8NZks5vhyIkgaJpZM4bx-b6>
.
|
|
We use "yargs-parser" to read CLI-argv plus all config files. Mocha has its own wrapper function
|
|
Looks like Edit: Here's relevant issue: #3965 |
|
@cspotcode You've got merge conflicts. |
|
@soryy708 If the maintainers say they want to merge this change, I'll resolve the merge conflicts. Until then, it's not worth the potentially wasted effort. |
|
I am a bot that watches issues for inactivity. |
Requirements
Description of the Change
This removes the yargs-unparser dependency and avoids loading yargs within
_mochaunless necessary. I still need to double-check these results, but it seems to reduce the installed size from 12M to about 6.6M.Alternate Designs
Why should this be in core?
Faster startup and install time; simpler runtime behavior because yargs is only running once.
Benefits
Faster startup and install time; simpler runtime behavior because yargs is only running once.
Possible Drawbacks
Applicable issues