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

Rewrite in Ink #83

Closed
wants to merge 28 commits into from
Closed

Conversation

jopemachine
Copy link
Contributor

@jopemachine jopemachine commented Mar 9, 2022

Fixes #34.

It seems working now but I think I need to a little more time to look into more error handling logics and test codes and lint (WIP).
Thank you for all your efforts for maintaining awesome libs.

@jopemachine
Copy link
Contributor Author

jopemachine commented Mar 9, 2022

  • This PR adds react, babel to dependencies to rewrite in ink, and removes inquirer from dependencies.
  • Some features not having unit tests were tested by myself in macos.
  • Lots of existing logic in interactive.js are moved to utils.js to separate UI codes with process fetching, searching codes.
  • I tried to maintain fkill-cli's existing action as possible as I can, but some actions might be changed due to using ink-select-input, ink-text-input. If this couldn't be accepted, please let me know. I will try to make the components instead of using them.
  • Some style (including text color) might have been changed a little bit.

@jopemachine jopemachine marked this pull request as ready for review March 9, 2022 12:34
@sindresorhus
Copy link
Owner

sindresorhus commented May 2, 2022

When searching, if you write a word, you should be able to press option+left-arrow to go back to the start, or option+right-arrow to go to the end. This worked before this PR.

Pressing Option+Delete when at the end of the search line, should clear the search text too.

@sindresorhus
Copy link
Owner

In case you are interested in making some improvements over the old version too:

  • The "(Move up and down to reveal more choices)" text should only be shown on the first launch (of the interactive UI). Same with "(Use arrow keys or type to search)".
  • You should be able to search for a port using the :8080 syntax. I was sure this was already supported, but does not seem to work in the latest version.

package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
.gitignore Outdated Show resolved Hide resolved
cli.js Outdated Show resolved Hide resolved
interactive.js Outdated Show resolved Hide resolved
interactive.js Outdated Show resolved Hide resolved
.github/workflows/main.yml Outdated Show resolved Hide resolved
package.json Show resolved Hide resolved
interactive.js Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
@jopemachine jopemachine marked this pull request as draft May 2, 2022 05:35
@sindresorhus
Copy link
Owner

@jopemachine Still interested in finishing this?

@jopemachine
Copy link
Contributor Author

jopemachine commented Nov 20, 2022

@jopemachine Still interested in finishing this?

Yes, I'm definitely interested in this issue and other current issues of fkill-cli.

So, there are some issues to look into.



  • Require statement issue
  • When searching, if you write a word, you should be able to press option+left-arrow to go back to the start, or option+right-arrow to go to the end. This worked before this PR.
  • Pressing Option+Delete when at the end of the search line, should clear the search text too.

: For implementing these features, I think updating ink-text-input, ink-select-input is required, but I don't think I can expect some reply at this point. I'm wondering what you think about how to handle this. What do you think about removing these libs and reimplementing this logic? Could I get some advice about this?


  • You should be able to search for a port using the :8080 syntax. I was sure this was already supported, but does not seem to work in the latest version.

: I think I can look into it when available.

@sindresorhus
Copy link
Owner

#83 (comment), I'm not sure about this.. Is this issue resolved now?

Check the output whether it's ESM or CJS. You may have set the modules option per the Babel comment.

@sindresorhus
Copy link
Owner

#83 (comment)

Just add a TODO comment about fixing it when Ink properly supports ESM.

@sindresorhus
Copy link
Owner

When searching, if you write a word, you should be able to press option+left-arrow to go back to the start, or option+right-arrow to go to the end. This worked before this PR.
Pressing Option+Delete when at the end of the search line, should clear the search text too.

I think we can live without these for now. We'll just open new issues about fixing it at some point.

@sindresorhus
Copy link
Owner

You should be able to search for a port using the :8080 syntax. I was sure this was already supported, but does not seem to work in the latest version.

Yes, that should ideally work. Not sure when it broke.

@jopemachine
Copy link
Contributor Author

Check the output whether it's ESM or CJS. You may have set the modules option per the Babel comment.

I'm not sure what is the Babel comment you mentioned, although the output is ESM.

Just add a TODO comment about fixing it when Ink properly supports ESM.

I think this is not required since ink-text-input, ink-select-input are not used now.

I think we can live without these for now. We'll just open new issues about fixing it at some point.

I copy and pasted the above two repositories' source code and patched the ink-text-input for support delete, option key features in b157fd6.

Yes, that should ideally work. Not sure when it broke.

I'm not sure about the cause but I checked this feature worked normally in 5.2.0, but broke in 6.0.0, maybe inquirer 7's default behavior change? I added the feature since I don't see port-searching related code.

I'd appreciate your letting me know if there's anything else I could help.

@jopemachine jopemachine marked this pull request as ready for review November 23, 2022 05:10
@sindresorhus
Copy link
Owner

Can you fix the merge conflict?

"execa": "^5.1.1",
"get-port": "^6.0.0",
"noop-process": "^5.0.0",
"process-exists": "^4.1.0",
"xo": "^0.45.0"
"xo": "^0.48.0"
Copy link
Owner

Choose a reason for hiding this comment

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

Upgrade to latest of all dependencies.

@sindresorhus
Copy link
Owner

Write foo in the prompt and then hold Option key while pressing Delete. The cursor moves backwards, but the text is not deleted. The expected behavior is that the whole text is erased after on press of Delete.

If you do the same, but hold Command instead Option, it adds a u character.

@sindresorhus
Copy link
Owner

Bump

@jopemachine
Copy link
Contributor Author

jopemachine commented May 1, 2023

@sindresorhus
I am still interested in finishing this PR, I was struggling with the key code testing logic on the cross-platform.
Personally, I have been so busy with work that I apologize for the late response.
If there is anyone interested in this PR, I would really appreciate it if you could finish the work.
I will try to find time to complete this PR, but I am not sure exactly when that will be.

@sindresorhus
Copy link
Owner

No worries. But since there's no definite date of that happening, I prefer to close this for now to keep my PR queue clean. Feel free to submit a new pull request if/when you have time to finish this.

@sindresorhus sindresorhus mentioned this pull request Jun 20, 2023
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.

Rewrite in Ink
2 participants