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

chore(rxjs): update rxjs to 6.3.2 #12357

Closed
wants to merge 1 commit into from

Conversation

benlesh
Copy link
Contributor

@benlesh benlesh commented Sep 24, 2018

Updates CLI to 6.3.2

NOTE: There were two typings that needed to be updated here. One was an honest fix due to improved typings and the limitations of TypeScript, the other is a bit more curious (See the changes to base.ts). There's apparently a problem with spreading an array of operators into pipe, but only in this case. I'm working on replicating the issue outside of this project, but I haven't been able to do so.

@benlesh benlesh requested a review from hansl September 24, 2018 18:26
@benlesh benlesh added the target: patch This PR is targeted for the next patch release label Sep 24, 2018
@@ -69,7 +69,7 @@ function _visitJsonRecursive<ContextT>(
? value as Observable<JsonValue>
: observableOf(value as JsonValue)
).pipe(
concatMap((value: JsonValue) => {
concatMap((value): Observable<JsonValue> => {
Copy link
Contributor

Choose a reason for hiding this comment

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

That’s not valid TypeScript is it?

Copy link
Contributor

Choose a reason for hiding this comment

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

It's missing the input type, or doesn't it need it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's what Rado recommended in these situations. Unfortunately, TS can't infer the return in these cases where there's branching code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The input type is inferred.

@alan-agius4
Copy link
Collaborator

@benlesh Thanks for this. However the E2E's will fail due to an issue that we had a chat about some time ago

ReactiveX/rxjs#4135

@@ -11,6 +11,7 @@
"schematics": "./collection.json",
"dependencies": {
"@angular-devkit/core": "0.0.0",
"@angular-devkit/schematics": "0.0.0"
"@angular-devkit/schematics": "0.0.0",
"rxjs": "6.3.2"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why does this need to be added here? AFAIK schematics/schematics do not directly depend on rxjs

@@ -14,6 +14,6 @@
],
"dependencies": {
"@angular-devkit/core": "0.0.0",
"rxjs": "6.2.2"
"rxjs": "6.3"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Shouldn't this be 6.3.2 for consistence? As this will be resolved to 6.3.x

@benlesh
Copy link
Contributor Author

benlesh commented Sep 25, 2018

Closing in favor of #12362

@benlesh benlesh closed this Sep 25, 2018
@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 12, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
target: patch This PR is targeted for the next patch release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants