Skip to content

Conversation

@jkuri
Copy link
Contributor

@jkuri jkuri commented Jan 30, 2016

Closes #152 and various other issues installing and injecting 3rd party libs.

@jkuri
Copy link
Contributor Author

jkuri commented Jan 30, 2016

Btw, thanks to @Brocco for helping me with new prompt texts.

Copy link
Member

Choose a reason for hiding this comment

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

"with a/the package name"

missing "a/the"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for noticing @cironunes, I fixed it.

@filipesilva
Copy link
Contributor

It looks like there's a lot less tests now. What is the reasoning?
Just curious really.

@jkuri
Copy link
Contributor Author

jkuri commented Jan 31, 2016

Before it was default that ng install lib-name --auto-injection injected everything that was exported the library in bootstrap script automatically. That wasn't okay, because we (if) want only providers to be injected in bootstrap script. I was testing if providers, directives, pipes were injected in a separate tests.
It would be possible to test if a directives, pipes, components were injected in auto generated component, if I know how to automatically go through prompts.
Btw, now this task goes in other way around, first injecting what and where user choose and then at the end prompts if he want providers injected in bootstrap script (if any providers in library). I think thats more reasonable and make this command really useful.

@filipesilva
Copy link
Contributor

Got it. Lgtm then!

Copy link
Member

Choose a reason for hiding this comment

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

minor: you can improve it to:

var allPackages = { toInstall: [], toProccess: [] };

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Improved. Thanks for analyzing the code Ciro, if you find anything else please just add more notes.

Copy link
Member

Choose a reason for hiding this comment

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

Can move this line to the top and use existSync on it on line 49?

@cironunes
Copy link
Member

Just added a few minor style improvements. Otherwise LGTM

@jkuri
Copy link
Contributor Author

jkuri commented Jan 31, 2016

Fixed. Thanks.

@jkuri jkuri deleted the libs-fix branch February 23, 2016 10:50
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 9, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3rd party libs installer prompts misleading

3 participants