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

Move types into namespace so that they can be imported #258

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

G-Rath
Copy link

@G-Rath G-Rath commented Jan 22, 2020

This allows access to the types without being a breaking change.

@G-Rath G-Rath requested a review from g-plane January 22, 2020 19:10
@@ -1,6 +1,52 @@
import { EventEmitter } from "events";

interface BasePromptOptions {
Copy link
Author

Choose a reason for hiding this comment

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

This diff looks a bit odd b/c for git the smallest way to express the change is to actually move the start of the namespace and the classes above it to the top of the file, rather than moving the interfaces into the namespace.

So the visualisation of this diff is inverse to it's result :)

@G-Rath
Copy link
Author

G-Rath commented Mar 13, 2020

@doowb could I get a review on this? (I can't request reviews anymore due to the age 😬)

@doowb
Copy link
Member

doowb commented Apr 7, 2020

@G-Rath thanks for the PR. Sorry for not responding sooner. There are a lot of TypeScript related PRs and since I'm not as familiar with TypeScript, I've been trying to make sure I understand what type of affects the changes might have.

From your initial description, this one looks like it will just "add" functionality by exposing more of the Types and Interfaces through the Enquirer namespace.

I just merged in #266, which updated index.d.ts a little. Will you "rebase" with that, then fix the indentation so it's easy to see the types and interfaces that are inside the namespace. (I realize that the BasePrompt class declaration already had bad indentation, so it would be awesome if you caught that too).

Thank you.

@g-plane
Copy link
Member

g-plane commented Apr 8, 2020

There's an existing PR: #82. However, it's still not merged.

@G-Rath
Copy link
Author

G-Rath commented Apr 8, 2020

this one looks like it will just "add" functionality by exposing more of the Types and Interfaces through the Enquirer namespace.

Exactly. This isn't a breaking change :)

@G-Rath
Copy link
Author

G-Rath commented Apr 30, 2020

@doowb I've rebased this PR - let me know if you'd like any other changes :)

@G-Rath
Copy link
Author

G-Rath commented Oct 18, 2020

@jonschlinkert @doowb any chance of getting this reviewed & merged? While it sounds like #307 is going to greatly improve on things, it's a big PR that could require work for apps to update to when it is finally released.

Meanwhile this change is very simple, not breaking, and so should provide a good middle ground to improve TS support while waiting for #307 to land :)

@jonschlinkert
Copy link
Member

jonschlinkert commented Oct 21, 2020 via email

@G-Rath
Copy link
Author

G-Rath commented Apr 18, 2021

@jonschlinkert any update?

@jonschlinkert
Copy link
Member

Sorry for the delay, I'l merge and publish asap.

@G-Rath
Copy link
Author

G-Rath commented Jul 30, 2021

@jonschlinkert I hate to be a pain, but any chance of getting this merged & released? I'm kicking off a new TypeScript project that I'd like to use enquirer for, but without this change it's making it pretty hard

@kevinkhill
Copy link

It's been over a year :(

@kevinkhill
Copy link

Screenshot_20220705-065152.png

This is what I meant. Empty promises.

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.

None yet

5 participants