-
Notifications
You must be signed in to change notification settings - Fork 2.2k
feat(): support Angular and rxjs 6 #1625
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
Conversation
This comment has been minimized.
This comment has been minimized.
Co-authored-by: Yuki Kawamoto <[email protected]>
This comment has been minimized.
This comment has been minimized.
@jamesdaniels @davideast I'm curious how this PR differs and why it's being favored. #1582 was rejected for being too big, but this PR has 37 files changed. I'm also disappointed in the lack of communication from the team. I'm happy I didn't spend the time working on the PR because I would have been upset to learn that a PR was being worked on by an org member and therefore would supercede any community effort. Regardless, thank you. |
@jamesdaniels @robertbaker It's not being favored. James has been working on various AngularFire issues and we haven't communicated with each other on this one due to Google I/O. @jymdman handled the separate PRs for each module. Sorry! @jamesdaniels I did not notice that @jymdman handled the PRs for each module. Can we just remove the removal |
Can the status of angular and rxjs 6 be clarified please? |
@GaryB432 We are clarifying who is working the PR in #1582. Who ever takes the issue first will work on it. We will also check on the status to make sure it is implemented in a timely manner. In the meantime please use the compat library until we get fully up to speed. Update: Pipeable operators are in master. We need to get other PRs merged and we can get updated to RxJS 6. |
@robertbaker I just noticed that @jymdman sent in the PRs which were small and done by feature. This PR looks large because it includes the removal of |
@robertbaker sorry for making this look "preferred". I already merged in several of @jymdman's PRs which brought the modules up to speed with pipeable operators. For the most part here I just pulled in the pieces of @kawamoto's pull request that were still needed; largely just updating the paths, build, karma.conf, and getting the tests green and please note I gave him co-contributor status here. I did this simply so I could move faster in merging this work; not to squash any community effort. @davideast I'll put the work in to support database-depreciated rather than drop it. We can do that after proposing a change. |
@robertbaker @GaryB432 and yes, we're sorry for the lack of communication here; both @davideast and I have been very busy prepping for, speaking at, and staffing Google I/O. I'm just trying to jump on this now that we're in the clear. I want everything fixed up so I can cut a new RC Monday AM. |
This comment has been minimized.
This comment has been minimized.
1 similar comment
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
1 similar comment
This comment has been minimized.
This comment has been minimized.
Wow, googlebot does not handle branches with co-contributors well at all. |
This comment has been minimized.
This comment has been minimized.
1 similar comment
This comment has been minimized.
This comment has been minimized.
test/ng-build/index.js
Outdated
@@ -11,7 +11,7 @@ function packageAngularFire() { | |||
const res = spawnSync('sh', ['pack.sh']); | |||
console.log(`------------ FINISHED PACKAGING VERSION ${PACKAGED_VERSION} ------------`); | |||
console.log(`------------ INSTALLING VERSION ${PACKAGED_VERSION} ------------`); | |||
if (shell.exec(`cd ng5 && npm i firebase ../${PACKAGED_VERSION}`).code !== 0) { | |||
if (shell.exec(`cd ng5 && yarn i firebase ../${PACKAGED_VERSION}`).code !== 0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yarn add
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good catch
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Wow, this mega change gets pushed out in a release candidate?!?! |
@maguro AngularFire is a combination of Firebase, Angular, and RxJS. Within a short period of time all libraries had major upgrades with breaking changes. We were already in the middle of a release candidate and were forced to make the changes. We opted to update to the major releases for everyone before going final. If we went final before upgrading then we would have had to break immediately. |
Checklist
yarn install
,yarn test
run successfully? yesDescription
Adding rxjs and angular 6 support.
Dropped database-deprecated, since there were a lot of changed needed there and it's no longer worth maintaining IMO.Huge shout out to @jymdman for doing the ground work in each module.