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

(feat) 3rd party libs #135

Merged
merged 1 commit into from
Jan 25, 2016
Merged

(feat) 3rd party libs #135

merged 1 commit into from
Jan 25, 2016

Conversation

jkuri
Copy link
Contributor

@jkuri jkuri commented Jan 7, 2016

I got this working, but like the whole project this is a prototype and a lot of things needed to be done.
This is how it looks like

[tp]$ ng libs install ng2-cli-test-lib
version: 1.13.13
Installing 3rd party packages: ng2-cli-test-lib...
1 Packages successfully installed
[?] Would you like to inject the installed packages into your app automatically? Y
[?] What is the path to the file which bootstraps your app? /Users/jan/Desktop/tp/src/app.ts

[?] What would you like to inject from ng2-cli-test-lib?
1 Directive
2 Pipe
3 Provider
4 styleUrl
Enter value: 1
[?] Which Directive would you like to inject?
1 TestDirective
Enter value: 1
[?] Where would you like to inject it?
1 /Users/jan/Desktop/tp/src/app.ts
2 /Users/jan/Desktop/tp/src/app/tp.spec.ts
3 /Users/jan/Desktop/tp/src/app/tp.ts
Enter value: 3
Injecting Directive (TestDirective) to /Users/jan/Desktop/tp/src/app/tp.ts
Done.

[tp]$

I tested on a https://github.com/NathanWalker/ng2-cli-test-lib test library, which regarding to comments on #96 should be it.

@NathanWalker
Copy link
Contributor

@jkuri you are amazing!
Really nice work thank you!
We may tweak the flow a little bit but this is definitely what we are after. Bravo 👍

@jkuri
Copy link
Contributor Author

jkuri commented Jan 7, 2016

@NathanWalker no problem. Would you write some guide how the packages needs to be created?

@NathanWalker
Copy link
Contributor

@jkuri You nailed it for most part.
I will write a guide once we have consensus. I'll cc' others below that may want to offer other ideas.

Main thing I object to is:

What would you like to inject from ng2-cli-test-lib?

In general, I believe it should ask something like this before that:

Would you like to customize the injection of ng2-cli-test-lib? (N/y)

If n (default):

Please provide the path to the file which bootstraps your root component:

/path/to/bootstrap.ts

// cli appends all lib providers to bootstrap.

Please provide the path to the component where you want the (directives, pipes, styles, styleUrls, etc.) added?

/path/to/root/component.ts

// cli injects and annotates everything else that the lib makes available.

If 'y':
Then it would go through the flow you have at the moment, which is more pick/choose (manual install) from the library.

I believe the default should be the easiest most painless path, which would be:
Add the lib, inject and annotate with everything the lib provides and don't allow the user to pick/choose

Path of least resistance I believe is adding providers to the apps bootstrap method ensuring it's available app-wide. This will help devs out of the gate I believe. More advanced users can answer y to Would you like to customize the injection of ng2-cli-test-lib? (N/y) to go the pick/choose (more power) route.

/cc @jvandemo, @gdi2290, @valorkin, @ocombe, @robwormald

@jkuri
Copy link
Contributor Author

jkuri commented Jan 8, 2016

My last commit allows auto import (this is default option) of everything that lib provide in bootstrap script. It also injects providers in bootstrap function.

@NathanWalker
Copy link
Contributor

@jkuri this is exciting! I can't thank you enough, I can't wait for this to land....

@IgorMinar please, pretty please? :)

lgtm. It can be improved over time if need be and the flow can continue to be tweaked, but having this now will allow lib authors to start advocating angular cli as a way to install their lib 👍

@jkuri
Copy link
Contributor Author

jkuri commented Jan 8, 2016

@IgorMinar yes, that would be cool :-). This command/task does not require any new dependencies, thanks to inquirer is built in ember-cli.

@filipesilva
Copy link
Contributor

Hey @jkuri, could you rebase this on master?

@valorkin
Copy link
Contributor

need to check my modules to work with this

@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.


  • If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
  • If you signed the CLA as a corporation, please let us know the company's name.

@jkuri
Copy link
Contributor Author

jkuri commented Jan 23, 2016

Hi @filipesilva, I rebased this.

@googlebot
Copy link

CLAs look good, thanks!

@jkuri
Copy link
Contributor Author

jkuri commented Jan 23, 2016

@valorkin use this script with systemjs-builder and run it in prepublish script. Also, here is example how to export things.

@hansl
Copy link
Contributor

hansl commented Jan 23, 2016

@jkuri: Hey Jan, good job!

  1. any reason why ng libs install foo was picked over ng install foo? I personally prefer smaller command lines. Seems like install could be purposed for this. I'm going to comment on Proposal: Auto add (providers, directives, pipes, styles, styleUrls) when adding 3rd party libs, etc. #96 about this.
  2. please squash your history before merging. We want to keep a clean commit history (same code practice as other angular projects). If you need help, you can check here and/or ask me!

The code looks good but I'm going to read it more thoroughly later on today.

@jkuri
Copy link
Contributor Author

jkuri commented Jan 23, 2016

Hi @hansl. Thank you!

Yes, I do need some help with squashing commits into one.. here's what I am trying to do
git rebase -i HEAD~9
then I mark all but first one with s and the first one I left with pick option. Then it rebases my commits, but when trying to push to a 3rd-party-libs branch it says my current branch is behind. Any suggestions?

I also like shorter commands but in that case I choose ng libs for two reasons. First is that when you type libs you know that you will install 3rd party library, and the second is that correlated install and uninstall commands can be merged into one ember-cli task.

@NathanWalker
Copy link
Contributor

@jkuri Whenever you squash commits, you are rewriting history ;)
So you have to force push:

git push -f

@hansl
Copy link
Contributor

hansl commented Jan 23, 2016

Unfortunately, you need to force push at this point.

@jkuri
Copy link
Contributor Author

jkuri commented Jan 24, 2016

Thanks. Commits squashed.

@googlebot
Copy link

We found a Contributor License Agreement for you (the sender of this pull request) and all commit authors, but as best as we can tell these commits were authored by someone else. If that's the case, please add them to this pull request and have them confirm that they're okay with these commits being contributed to Google. If we're mistaken and you did author these commits, just reply here to confirm.

@googlebot
Copy link

CLAs look good, thanks!

@cironunes
Copy link
Member

This is pretty cool! Thanks @jkuri.

I'd do 2 things before merging this in:

  • Change the command to ng lib install (as we are installing one lib per time, I think that keeping it in singular make more sense) or ng install as suggested by @hansl
  • Add unit tests for the feature

@jkuri
Copy link
Contributor Author

jkuri commented Jan 24, 2016

@cironunes thanks, no problem.
You can install/uninstall multiple libs per time, separating them by space.
ng install and ng uninstall as @hansl suggested will be good, but then I need to break up the task into two separated tasks. Also ng i and ng u will then be possible as aliases to this commands, even shorter. Should I implement it in that way?

@hansl
Copy link
Contributor

hansl commented Jan 24, 2016

@jkuri: I personally don't see it as a blocker for this PR. It can easily be done in a separate PR. I checked and the ember install currently fails so people won't be using it.

You're 👍 to commit.

@jkuri
Copy link
Contributor Author

jkuri commented Jan 24, 2016

@hansl ok, I agree. I can override the core install command, but if I remember correctly it logs some warning that the command has been overridden. Will check if I can take care of that.

@NathanWalker
Copy link
Contributor

@jkuri not a huge deal, but you might reword your commit msg from:

(feat) 3rd party libs

to:

feat(libs): 3rd party lib helper

closes https://github.com/angular/angular-cli/issues/96

@hansl
Copy link
Contributor

hansl commented Jan 24, 2016

And please squash your history. I can take time right off now to help you merge if you need to.

@jkuri
Copy link
Contributor Author

jkuri commented Jan 24, 2016

Thanks, I managed to squash it yesterday, I will make another commit when separating commands later today. I was having problem with squashing merged stuff from master. It will not harm if you write commands in order, it can save me some time.

@hansl
Copy link
Contributor

hansl commented Jan 24, 2016

I would suggest doing a separate PR just so we can get this in. Also, I feel this second commit came from a git merge; you should rather do git rebase master instead (assuming your master is up to date). It doesn't commit anything (although you have to git push --force)

@NathanWalker
Copy link
Contributor

git checkout master
git fetch upstream
git rebase upstream/master
git checkout 3rd-party-libs
git rebase master  // if you have merge conflicts, you may need to fix and add, then `git rebase --continue`
git rebase -i HEAD~4  // reword, squash and do what you need to do
git push -f

@jkuri
Copy link
Contributor Author

jkuri commented Jan 25, 2016

@hansl and @NathanWalker thanks for your help with that. I squashed commits without any problems now.

I reword commands to ng install <name-of-lib> and ng uninstall <name-of-lib> and put them in separate tasks. Here is one variant of how install process looks like, and here is screenshot of uninstall process. All the things gets properly injected and also cleaned after uninstalling, but I still need to write unit tests.
After this will be merged, we should write documentation about how the lib needs to be created to be compatible with our commands, so developers can start building/reconfigures their libs the right way.

@NathanWalker
Copy link
Contributor

Agreed, I will help write a lib guide over next 2 weeks (prob become its
own repo with wiki and all) that we can collab on to make good docs for how
to author libs that will work with this feature. Again, really nice work
here.

On Sun, Jan 24, 2016 at 5:43 PM Jan Kuri notifications@github.com wrote:

@hansl https://github.com/hansl and @NathanWalker
https://github.com/NathanWalker thanks for your help with that. I
squashed commits without any problems now.

I reword commands to ng install and ng uninstall
and put them in separate tasks. Here
https://cloud.githubusercontent.com/assets/1796022/12540605/bfb40d96-c30b-11e5-8397-5f0ca544c417.png
is one variant of how install process looks like, and here
https://cloud.githubusercontent.com/assets/1796022/12540653/8133b142-c30c-11e5-8e53-d009620bd027.png
is screenshot of uninstall process. All the things gets properly injected
and also cleaned after uninstalling, but I still need to write unit tests.
After this will be merged, we should write documentation about how the lib
needs to be created to be compatible with our commands, so developers can
start building/reconfigures their libs the right way.


Reply to this email directly or view it on GitHub
#135 (comment).

@hansl
Copy link
Contributor

hansl commented Jan 25, 2016

If we do documentation I suggest keeping it under a single roof. I don't mind either creating a doc/ folder with markdown files or using this github's wiki.

@NathanWalker
Copy link
Contributor

Starting with the wiki here sounds like a good idea.

On Sun, Jan 24, 2016 at 5:53 PM Hans notifications@github.com wrote:

If we do documentation I suggest keeping it under a single roof. I don't
mind either creating a doc/ folder with markdown files or using this
github's wiki.


Reply to this email directly or view it on GitHub
#135 (comment).

@jkuri
Copy link
Contributor Author

jkuri commented Jan 25, 2016

Added tests to both commands.

@cironunes
Copy link
Member

There you go. Awesome job @jkuri. LGTM

@filipesilva
Copy link
Contributor

Awesomesauce. Merging it then!

@filipesilva filipesilva merged commit 0195146 into angular:master Jan 25, 2016
@jkuri
Copy link
Contributor Author

jkuri commented Jan 25, 2016

Thanks for helping guys.

@jkuri jkuri deleted the 3rd-party-libs branch January 25, 2016 17:37
@NathanWalker
Copy link
Contributor

Thank you to everyone here. @jkuri incredible effort. If you are on Twitter, you might DM me, we can chat about further details of documentation coordination, etc (@wwwalkerrun)

@ocombe
Copy link
Contributor

ocombe commented Jan 25, 2016

\o/

Right on time for my talk at NG-NL on "How to write libraries for Angular 2" :)

@filipesilva
Copy link
Contributor

Sweet. Tells us how it goes on gitter! https://gitter.im/angular/angular-cli

@cironunes
Copy link
Member

can't wait for the next release :)

@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.

None yet

8 participants