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

feat(cli): display "Happy Hacking" message at the end of the project creation #178

Merged
merged 13 commits into from
Apr 29, 2021

Conversation

y-lakhdar
Copy link
Contributor

image

  • Making sure the "Happy Hacking" message is displayed at the complete end of the project creation
  • Added a spinner for the token server creation. Otherwise there's a couple of seconds without any feedback

@github-actions
Copy link
Contributor

Pull Request Report

PR Title

✅ Title follows the conventional commit spec.

@@ -16,6 +17,7 @@ import {
IsNpxInstalled,
} from '../../../lib/decorators/preconditions';
import {appendCmdIfWindows} from '../../../lib/utils/os';
import {EOL} from 'os';
Copy link
Collaborator

@louis-bompart louis-bompart Apr 22, 2021

Choose a reason for hiding this comment

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

  • Test on Windows. (I'll do it)

Copy link
Collaborator

@louis-bompart louis-bompart left a comment

Choose a reason for hiding this comment

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

Can you try the buffer? I'm curious ;)
Also, I think we could improve the E2E for react in that sense to detect the 'Happy HAcking!' as a exit signal, as we did for the others.

Apart from that great job!

Tho, PSA y'all:
⚠️ microsoft/node-pty is a node native dependency ⚠️
We should build and test the binary.

packages/cli/src/commands/ui/create/react.ts Outdated Show resolved Hide resolved

const args = this.args;
let stopWritingInTerminal = false;
let remainingString = '';
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think that if you create a buffer and write in it, it might works without --color=always and be more efficient :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oups. The --color-always was added as an initial tentative to preserve the colors. Now that we use node-pty, I don't need it.

name: 'xterm-color',
cols: process.stdout.columns,
rows: process.stdout.rows,
cwd: options.cwd || process.cwd(),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Use ?? instead of ||. cwd:'' is valid IMO

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good catch

@y-lakhdar
Copy link
Contributor Author

y-lakhdar commented Apr 23, 2021

Looks like it broke the E2E tests.

  • Update E2E tests
  • Build and test node-pty native dependency

@louis-bompart
Copy link
Collaborator

Looks like it broke the E2E tests.

  • Update E2E tests
  • Build and test node-pty native dependency

Just to be clear about the node-pty I'm not saying we should built it beforehand ourselves on our machine. This is the CD job.
However we ought to test it ;)

@y-lakhdar y-lakhdar requested a review from louis-bompart April 26, 2021 12:12
@louis-bompart
Copy link
Collaborator

I'm building binaries from this branch to ensures all's well:
https://github.com/coveo/cli/actions/runs/785699814

@louis-bompart
Copy link
Collaborator

louis-bompart commented Apr 26, 2021

image
Can you compare the behavior between current release and the new one?
coveo ui:create:react reactTestPty
made this one

The behaviour constated:

  • Created Server OK is odd
  • Terminal is not released (I suppose we still wait for the Happy Hacking message or something from React (oops)

Expected behaviour:

  • Don't try to create the server if 💩 goes wrong before.
  • Ensure process exit swiftly when cra fails.

@louis-bompart
Copy link
Collaborator

When using a valid process name, the terminal/stdin is not released :/

Copy link
Collaborator

@louis-bompart louis-bompart left a comment

Choose a reason for hiding this comment

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

Required changes:

  • When create-react-app fails, we should promptly exit.
  • When the command complete, it should release the prompt.

@y-lakhdar y-lakhdar merged commit c5e7619 into master Apr 29, 2021
@louis-bompart louis-bompart deleted the CDX-225 branch July 29, 2021 16:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants