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

New command line help developer experience. #44157

Merged
merged 28 commits into from
Jun 16, 2021

Conversation

ShuiRuTian
Copy link
Contributor

Fixes #44074

@typescript-bot typescript-bot added the For Uncommitted Bug PR for untriaged, rejected, closed or missing bug label May 19, 2021
@ShuiRuTian
Copy link
Contributor Author

ShuiRuTian commented May 19, 2021

Still to do things:

  • Add Typescript Icon
  • Check whether the amount of line break is correct
  • plugins shold be in basic category.
  • ...

@ShuiRuTian
Copy link
Contributor Author

ShuiRuTian commented May 19, 2021

Issues:

  1. moduleresolution description of default is changed. I could not find a way to produce line break in Diagnostics
from:
`module === AMD or UMD or System or ES6, then Classic

Otherwise Node`
to:
`module === AMD or UMD or System or ES6, then Classic, Otherwise Node`
  1. incremental description of default is changed. This is more consistent with other descriptions.
from:
"`true` if `composite`, `false` otherwise",
to:
"`false`, unless `composite` is set"
  1. Add plugin option to Basic Option category, before this PR, it does not belong to any category!
  2. As we have any of to tell user candidate values, some description could be changed now. --jsx from Specify JSX code generation: 'preserve', 'react-native', 'react', 'react-jsx' or 'react-jsxdev'. to Specify JSX code generation.. The similar change is applied to target and module

@orta
Copy link
Contributor

orta commented May 19, 2021

Very cool work! (output)

  1. I think if an option is a boolean and the default: is false or undefined or n/a, we can probably drop the 'default' and 'any of' . Might make it a bit less sparse.
  2. There's one or two fasles typos in there
  3. Wow there's a lot of lib options!
  4. Maybe we should drop a few more options from the default. Let me poke some bears about that to see what others think.

@ShuiRuTian ShuiRuTian marked this pull request as ready for review May 22, 2021 16:27
@ShuiRuTian
Copy link
Contributor Author

ShuiRuTian commented May 23, 2021

There is a strange lint error. I feel it is not related to this PR.


edit: weird! The output of azure and github action tells out of memory. Need some time to find out the reason.

@orta
Copy link
Contributor

orta commented May 27, 2021

Great work!

I'll tweak this a bunch and see if I can get the linter not bailing too

@sandersn sandersn requested a review from orta May 28, 2021 21:31
@orta
Copy link
Contributor

orta commented Jun 2, 2021

Confirmed with some logs in the test that there an infinite loop, will look at this tomorrow

@ShuiRuTian
Copy link
Contributor Author

Finally, I moved into new house and have spared time....

And thanks a lot to @orta 's analytics, it is really helpful.
I found the reason is about child_process, gulp runtests could finish normally, but gulp runtests-parallel cause infinite loop.
In parallel test, process.stdout.columns is undefined.

The related code is in host.ts

process: fork(__filename, [`--config="${configPath}"`], { stdio: ["pipe", "pipe", "pipe", "ipc"] }),

It is easy to test, we could create two files:
tmp.js (main thread)

const {
    Worker, isMainThread, parentPort, workerData,
} = require("worker_threads");

const { fork } = require("child_process");

console.log("main thread: " + __filename);

const child = fork(`${__dirname}/tmp2.js`, [], { stdio: ["pipe", "pipe", "pipe", "ipc"] });

child.stdout.setEncoding("utf8");
child.stdout.on("data", function (data) {
    console.log(data);
});

tmp2.js (child process)

console.log("worker thread stdout columns: "+process.stdout.columns);

The output is worker thread stdout columns: undefined. Try to fix it....

@ShuiRuTian
Copy link
Contributor Author

Auh, it seems a little strange for the test case of help now:

  1. gulp runtests and gulp runtests-parallel produce different result, please see my last comment.
  2. We use the width of terminal to give a pretty output, but the width is pretty easy to be changed, which means gulp runtests will produce a difference expected output for this case! And the test just failed...

What should we do? I feel we need to delete this case, the correspond file is runWithoutArgs.ts

@orta
Copy link
Contributor

orta commented Jun 3, 2021

I wonder if we can hardcode the width in the test somehow?

@ShuiRuTian
Copy link
Contributor Author

Good suggestion!
And the merge conflict is resolved, we could focus on output now.

src/compiler/sys.ts Outdated Show resolved Hide resolved
src/harness/fakesHosts.ts Outdated Show resolved Hide resolved
src/harness/harnessLanguageService.ts Outdated Show resolved Hide resolved
src/harness/virtualFileSystemWithWatch.ts Outdated Show resolved Hide resolved
src/testRunner/unittests/tsserver/session.ts Outdated Show resolved Hide resolved
src/webServer/webServer.ts Outdated Show resolved Hide resolved
Copy link
Member

@sheetalkamat sheetalkamat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed only API and not actual functionality and that looks good now.

@orta
Copy link
Contributor

orta commented Jun 8, 2021

I've done some work on getting this ready locally, it's close - I've got to set up all the localization etc. Hopefully should be ready tomorrow.

@orta orta changed the title refactor help command line output. New command line help developer experience. Jun 9, 2021
Copy link
Contributor

@orta orta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright, this is looking good to me. I'll give it till next week to give some other compiler folks to take a look over the changes but thanks a lot @ShuiRuTian! Great work.

@orta
Copy link
Contributor

orta commented Jun 16, 2021

Alright, let's go.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
For Uncommitted Bug PR for untriaged, rejected, closed or missing bug
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Revise TypeScript CLI help documentation
4 participants