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

downgrade inquirer #4519

Closed
wants to merge 1 commit into from
Closed

downgrade inquirer #4519

wants to merge 1 commit into from

Conversation

bugzpodder
Copy link

@bugzpodder bugzpodder commented May 25, 2018

#3880 (comment)

Basically removes rxjs dependency, which is around 18MB.
Tested yarn eject, yes/no.

@iansu
Copy link
Contributor

iansu commented May 25, 2018

I'm not sure if this is the best way to handle this. If we do this then we have to remember to not update this dependency in the future. Is there a lightweight alternative to Inquirer that we could use instead?

@bugzpodder
Copy link
Author

Yep, there definitely should be, will check later. I think you added the inquirer library over prompt.js #1772
the worst case scenario is that we can bring that piece of code back?

@iansu
Copy link
Contributor

iansu commented May 25, 2018

That's right. I think that was my first contribution to Create React App. 😀

I'd like to avoid going back to a custom solution. There was a reason we moved away from that in the first place. I'll look around a bit too and see if I can find a good alternative. I suppose we could also raise an issue with Inquirer if someone hasn't already.

@bugzpodder
Copy link
Author

closing this in favor of a smaller package.

@bugzpodder bugzpodder closed this May 25, 2018
@bugzpodder bugzpodder deleted the package branch May 25, 2018 17:56
@bugzpodder
Copy link
Author

@iansu finally found something reasonable:
If we just want Y/n prompts:
https://github.com/enquirer/prompt-confirm
If we want a more inquirer like experience:
http://enquirer.io/

@wtgtybhertgeghgtwtg
Copy link
Contributor

Asking inquirer to use a different Observable implementation is probably a better bet. eslint@5 is using newer versions of inquirer, so it'll come back, anyway.

@bugzpodder
Copy link
Author

bugzpodder commented May 27, 2018

hmmm. they upgraded from rx-lite (400kb) to rx5 and more recently rx6.
SBoudrias/Inquirer.js#614

@iansu
Copy link
Contributor

iansu commented May 28, 2018

@bugzpodder Do you mind opening an issue with Inquirer about reducing their install size? Let's see what they say before we consider replacing the package.

@bugzpodder
Copy link
Author

Sure I'll see if i can open a few issues.
From my understanding I think they are set on using rxjs and it probably doesn't make sense for them to switch to another library. SBoudrias/Inquirer.js#614
It is a problem with RxJs itself where it is including src/ in the npm bundle. ReactiveX/rxjs#3334
I assume this will be eventually fixed by rxjs.
I also tested enquirer and its api is mostly identical to inquirer so it might be okay to just switch and deal with it later. There are a few odd UI issues that I can just submit a one-line PR for (eg showing: Do you want to eject: true instead of Do you want to eject: Yes) . but library otherwise is mostly pretty solid.

@iansu
Copy link
Contributor

iansu commented May 28, 2018

It looks like RxJS is intentionally publishing src. Let's see if we can get this fixed upstream and if not then we can discuss replacing Inquirer.

@bugzpodder
Copy link
Author

bugzpodder commented May 28, 2018

The src is about 2.2MB, so its unclear if excluding source will help that much.

1.7M	./operators
384K	./util
188K	./scheduler
 36K	./symbol
 96K	./testing
4.0M	./_esm5
1.5M	./add
3.9M	./_esm2015
924K	./observable
1.2M	./operator
2.4M	./bundles
2.2M	./src
 19M	.

@lock lock bot locked and limited conversation to collaborators Jan 19, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants