Skip to content

Full rewrite for Ink 2, Hooks, & Typescript#4

Open
john-osullivan wants to merge 11 commits intoaicioara:masterfrom
Eximchain:master
Open

Full rewrite for Ink 2, Hooks, & Typescript#4
john-osullivan wants to merge 11 commits intoaicioara:masterfrom
Eximchain:master

Conversation

@john-osullivan
Copy link

All of the existing examples were migrated over as well! If you're interested in trying to merge this in, I'll remove my README changes from this PR. Not sure if you'd be interested in using Typescript here; I prefer it to setting up all the other JS tooling, but everyone has their own preference.

Happy to make a gif of the input in action if it'd give you some peace of mind!

@aicioara
Copy link
Owner

Oh, wow, thanks @john-osullivan . This PR is unexpected. I am in between a few major projects right now, so I might need some time to context switch back to ink-quicksearch and review the PR, especially given its nature, but I assure you that it is on my TODO list now and will get back to it as soon as I can.

Few questions:

  • Is the new version still backwards compatible with Ink v1?
  • Does the old version work with Ink v2?
  • What was the main motivation for the rewrite?

I had a quick glance at the code and it looks more like a hint for me to apply to my codebase rather than a PR that can be merged as-is. I am referring to the module renames, package.json metadata, readme (which you mentioned), etc. Is that accurate? I am trying to gauge the intention behind the PR.

Thanks again and I look forward to hearing from you!

@john-osullivan
Copy link
Author

john-osullivan commented Jan 21, 2020

Hallo! No rush, I've already got my fork plugged into my project. To your questions:

  1. No, I don't believe that components can be cross-compatible from Ink v1 to v2. v2 introduced different primitive elements and stopped overriding React's Component definition; pretty sure it's a breaking change.
  2. Same as above.
  3. My big motivation was using the functionality in an Ink 2 CLI, and also having typing built directly into the package. I've become a huge fan of Typescript over the last year, and after working with all of its conveniences, it's tough to go back.

I think you're right that it isn't worth trying to merge this re-write, as it's such a big diff. I didn't go through the effort of cleaning out my fork changes because I wasn't sure if you'd be interested in bringing all of this in. Once you get a chance to read it over, let me know if you'd like to start using this implementation and I'll bring everything back in line with your original package!

Edit: re using hooks, I'm not sure if this counts as a benefit, but in my first rewrite attempts, I ran into some bugs making hooks play nice with class components. Hooks can only be called in FunctionComponents; when I passed a hook setter into a class component and then the class component called it, I ran into that "Hooks can only be called from FunctionComponents" error. So using hooks isn't necessarily a huge upgrade for this library itself, but it makes it play nicer with the evolving React ecosystem.

@aicioara
Copy link
Owner

Cool. Thank you for your message and for your interest in bringing this change in.

  • I am totally interested in bringing this change in
  • I believe it is not good practice (or even possible) to use hooks with class components. I was interested more in the "can we make this work without hooks at all". I believe the answer is yes, and if so, I would like to avoid hooks, as that would make the rewrite easier to review
  • The idea of typescript is interesting and I hear you. I used it extensively when developing https://github.com/palantir/plottable and I am confident that its positives outweight its negatives. However, ink is fully written in jsx and I want plugins to stay in that ecosystem. I want to allow typescript consumers through a d.ts file (like ink does), but I think there is a lot of value in keeping everything in jsx

Let me know your thoughts and if you are happy (and have the time) to clean this up as a non-hook, non-ts, yes-d-ts upgrade, I am super happy to merge. I also really appreciate you bringing this to my attention. We can review hooks as a subsequent PR.

Thanks!

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.

2 participants