Skip to content

feat(schematics): ng-add schematics #1853

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

Closed
wants to merge 1 commit into from

Conversation

sulco
Copy link
Contributor

@sulco sulco commented Sep 2, 2018

Checklist

  • Issue number for this PR: Feat: ng add angularfire2 #1676 (required)
  • Docs included?: yes (yes/no; required for all API/functional changes)
  • Test units included?: no (yes/no; required)
  • In a clean directory, yarn install, yarn test run successfully? yes (yes/no; required)

Description

This adds ng add command that simplifies adding and setting up AngularFire to Angular apps.
It follows the practices described in the current setup instructions. I've also updated these to include the ng-add command (the "manual" instructions are still left available though).

Code sample

To try this functionality out, in the project's folder run:

  • npm run build
  • cd dist/packages-dist
  • npm link

Then create a sample Angular project wherever (ng new angularfire-dummy) and in that directory run

  • npm link @angular/fire
  • ng add @angular/fire

This is my first contribution here but I've tried to be careful about preparing this - if you find anything unclear though, or in a need of change, please do bug me;)

@sulco sulco force-pushed the feature/schematics_ng-add branch from 27e48e8 to 00f3a6e Compare September 2, 2018 20:46
@sulco sulco changed the title Feature/schematics ng add feat(schematics): ng-add schematics Sep 2, 2018
@sulco sulco force-pushed the feature/schematics_ng-add branch from 00f3a6e to ac6251c Compare September 2, 2018 20:49
@@ -2,6 +2,8 @@

> Using Ionic and the Ionic CLI? Check out these [specific instructions](ionic/cli.md) for Ionic and their CLI.

> Following instructions use `ng add` command that automates several steps for you. If you want to set up things manually refer to [these steps](install-and-setup-manual.md) instead.
Copy link
Contributor

@EdricChan03 EdricChan03 Sep 3, 2018

Choose a reason for hiding this comment

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

Typos:

  • "Following instructions" -> "The following instructions"
  • "use ng add command" -> "use the ng add command"
  • Add a comma to "If you want to set up things manually"


- `AngularFirestoreModule`
- `AngularFireAuthModule`
- `AngularFireDatabaseModule`
- `AngularFireStorageModule`
- `AngularFireMessagingModule` (Future release)

#### Adding the Firebase Database and Auth Modules
### 4. Edit Firebase config in environments variable
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably simpler to just say "Edit Firebase configuration"?

storageBucket: '<your-storage-bucket>',
messagingSenderId: '<your-messaging-sender-id>',
},
production: false,
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove the leading comma here

@@ -141,16 +81,14 @@ import { AngularFirestore } from 'angularfire2/firestore';
@Component({
selector: 'app-root',
templateUrl: 'app.component.html',
styleUrls: ['app.component.css']
styleUrls: ['app.component.css'],
Copy link
Contributor

Choose a reason for hiding this comment

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

Once again, remove the leading comma here.

@@ -162,7 +100,7 @@ import { Observable } from 'rxjs';
@Component({
selector: 'app-root',
templateUrl: 'app.component.html',
styleUrls: ['app.component.css']
styleUrls: ['app.component.css'],
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove leading comma here

"path": {
"type": "string",
"format": "path",
"description": "The path to the project.",
Copy link
Contributor

Choose a reason for hiding this comment

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

You should be more specific here. Do you mean the project's root, or something else?

@sulco sulco force-pushed the feature/schematics_ng-add branch from e989ad5 to e1dfe23 Compare September 3, 2018 09:50
@googlebot
Copy link

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

1 similar comment
@googlebot
Copy link

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

@googlebot googlebot added cla: no and removed cla: yes labels Sep 3, 2018
@sulco sulco force-pushed the feature/schematics_ng-add branch from e1dfe23 to a5e5129 Compare September 3, 2018 10:05
@googlebot
Copy link

CLAs look good, thanks!

@googlebot googlebot added cla: yes and removed cla: no labels Sep 3, 2018
@googlebot
Copy link

CLAs look good, thanks!

@sulco
Copy link
Contributor Author

sulco commented Sep 3, 2018

Thanks @Chan4077, I've made the requested changes.

@sulco sulco force-pushed the feature/schematics_ng-add branch 2 times, most recently from 8ce49cc to b85643d Compare September 3, 2018 13:29
@davideast
Copy link
Collaborator

Hey @sulco! Thank you so much for taking this task on! Unfortunately it's a little big of bad timing with our switch to the @angular/fire package. How about we wait until that lands and then we refactor this to use the new package name? Does that sound like a plan?

@sulco
Copy link
Contributor Author

sulco commented Sep 7, 2018

Hi @davideast! Yeah, I've seen that 😳Well, waiting is hard but actually leaving things undone is much worse, so full support to the plan. Let's wait for that landing and pick it up from there.

@sulco sulco force-pushed the feature/schematics_ng-add branch 2 times, most recently from 7f0d9a7 to 19b446e Compare September 9, 2018 15:36
@sulco
Copy link
Contributor Author

sulco commented Sep 9, 2018

Ok @davideast, I've adjusted the code with the @angular/fire changes (and also cleaned up the commits to more sensible 2)

@lundelius
Copy link

Hi Guys,
I just checked out the NG v7...

Would be lovely if you finish this PR for ng add @angular/fire !

@IOAyman
Copy link

IOAyman commented Nov 30, 2018

Thanks everybody for the good work :)

Any updates or ETA for this, please?

/cc @davideast @Chan4077 @sulco

@EdricChan03
Copy link
Contributor

Why the ping @IOAyman?

I'm not a collaborator of this project.

@sulco
Copy link
Contributor Author

sulco commented Nov 30, 2018

I can update this over the weekend so it doesn't break travis, and take a look if it still works after all these months, but other than that it's out of my hands.
It just needs that review, I'll probably just bug the maintainers more for it.

@IOAyman
Copy link

IOAyman commented Dec 2, 2018

@sulco That'd be great, thanks. Hopefully a collaborator will merge and release it soon.

@Chan4077 Sorry, my bad.

@Splaktar
Copy link
Contributor

It looks like this needs a rebase now. I'd love to see this get in soon so that it could get tweaked a bit more (in a separate PR) for Angular version 8.

A possibility to install and configure the package with `ng add angularfire2` command

closes angular#1676
@sulco sulco force-pushed the feature/schematics_ng-add branch from e775eea to 4d3b35d Compare March 31, 2019 16:59
@sulco
Copy link
Contributor Author

sulco commented Mar 31, 2019

Hey, @Splaktar! Apologies for taking so long and thanks for the encouragement - I've just rebased it, updated relevant dependencies to the latest + updated the PR's description on how to try the functionality out.

Hopefully we're closing in on this one! :)

"@angular/common": ">=6.0.0 <8",
"@angular/compiler": ">=6.0.0 <8",
"@angular/core": ">=6.0.0 <8",
"@angular/platform-browser": ">=6.0.0 <8",
"@angular/platform-browser-dynamic": ">=6.0.0 <8",
"bufferutil": "^3.0.3",
Copy link
Contributor

Choose a reason for hiding this comment

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

Just wondering, but what is bufferutil needed for here?

@sulco
Copy link
Contributor Author

sulco commented Apr 11, 2019

Thank you for pointing this one out @Chan4077 - tbh I don't remember, it's been so long since I've created this PR. I'll look that one up with once I gather the remaining feedback.

On this note. Are there any plans to have this reviewed and merged any time soon @Splaktar / @davideast? The new versions of - well, everything - are coming and going, and tbh I don't know if there is a point of just keep rebasing this PR every now and then ;)

@jamesdaniels
Copy link
Member

@sulco Thanks for your work getting these base schematics rolling. We're centering work around #2046 which includes a deploy command to Firebase Hosting. Would you be able to look at whether the schematics you've built here would able be to ported over to that branch / directory style?

@sulco
Copy link
Contributor Author

sulco commented May 14, 2019

@jamesdaniels Looking at https://twitter.com/mgechev/status/1128164567459274752 I'm not sure if this work is relevant anymore - looks like you've got ng-add functionality covered in the meantime.

Kinda disappointing tbh. Anyways, please close/reject/... this PR if that's the case. Doesn't make sense to keep this one hanging.

@Splaktar
Copy link
Contributor

Yes, I agree that not accepting or even reviewing this PR, letting it sit for 6+ months, and then re-doing the work in a separate branch/PR is NOT how open source should work. It does not provide a good experience to skilled contributors who spend a large amount of spare time to help the project.

I understand that resources to maintain this repo are very limited, but that's even more of a reason that work like this should have been leveraged.

@jamesdaniels please try to find a way to ensure that something like this doesn't happen again 😞 Additionally, please set expectations properly for contributors in the docs here and when you are at conferences. If PRs, or certain types of PRs, won't be reviewed or accepted, then that needs to be communicated in order to avoid wasted time, frustration, etc.

@jamesdaniels
Copy link
Member

I still believe there is value in this PR. As the ng-add and deploy commands as they exist on master don't overlap with the functionality of this PR. As mentioned on Apr 25th, I'd like to see if we can combine the efforts. Sorry for the lack of communication here, I too was caught off guard by the ng-deploy addition.

@jamesdaniels jamesdaniels added this to the 5.3.0 milestone May 31, 2019
@jamesdaniels
Copy link
Member

Also I don't believe communicating when I'm at conferences will be terribly useful, as that's pretty much my job 😝 think I'm on track to hit 30 this year.

TBH I thought @davideast had this one and that was a failure in our communications.

We are ramping up additional team members on the project and I simplified the release process with CI/CD (pending the Angular team allowing me to create the git triggers), which should help our agility.

@jamesdaniels jamesdaniels requested a review from davideast May 31, 2019 13:44
@Splaktar
Copy link
Contributor

Also I don't believe communicating when I'm at conferences will be terribly useful, as that's pretty much my job

I think that there was some kind of misunderstanding here. It sounds like you are responding to a request for you to let contributors know when you will be at conferences. That wasn't at all what I was meaning.

Let me try rephrasing it...

When you are at conferences (as you mentioned, frequently), please talk about the expectations that contributors to @angular/fire should have. It would be nice to also cover contribution expectations in the docs. If PRs, or certain types of PRs, won't be reviewed or accepted, then it would be nice to communicate that in order to avoid wasted time, frustration, etc.

@theTechGoose
Copy link

@sulco ng add is no longer working, please see #2285

@Splaktar
Copy link
Contributor

@Rofer11607 I don't think that this issue has anything to do with the changes in this PR by @sulco, which have not been merged.

@jamesdaniels jamesdaniels modified the milestones: 5.3.0, 6.1 Nov 11, 2020
@jamesdaniels jamesdaniels modified the milestones: 6.1, 6.2 Nov 18, 2020
@jamesdaniels jamesdaniels modified the milestones: 6.2, 7.1 Aug 25, 2021
@jamesdaniels
Copy link
Member

Closing in favor of #2836, where I brought over the feature initialization and environment config changes from this PR. Couple more things left to do but I'll include in 7.1, cutting a release candidate soon.

@jamesdaniels
Copy link
Member

@sulco sorry it took so long to get this one merged in. Thanks for your contribution, you've been co-authored in the big schematics commit.

@schematics/angular is now published, so I didn't have to bring in all that in.

In #2836 I've modernized with the new API & really amped up the ng add & firebase-tools integration so you can select/create projects, sites, and download the env config.

@sulco
Copy link
Contributor Author

sulco commented Sep 16, 2021

Oh snap, thanks @jamesdaniels! I think that that's the longest "better late than never" I've ever experienced :D
And thanks @Splaktar for caring about the OSS experience 🙌

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants