Skip to content

build: bump to angular 6.0.0-rc.1 and rxjs 6.0.0-rc.0 #10642

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Apr 3, 2018
Merged

build: bump to angular 6.0.0-rc.1 and rxjs 6.0.0-rc.0 #10642

merged 1 commit into from
Apr 3, 2018

Conversation

crisbeto
Copy link
Member

Bumps to the latest Angular and RxJS RCs and fixes a compilation error in the postinstall script.

@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Mar 31, 2018
crisbeto
crisbeto commented Mar 31, 2018
View reviewed changes
package.json Outdated
@@ -57,7 +57,7 @@
"@types/glob": "^5.0.33",
"@types/gulp": "3.8.32",
"@types/hammerjs": "^2.0.35",
"@types/jasmine": "^2.6.0",
"@types/jasmine": "2.2.34",
Copy link
Member Author

@crisbeto crisbeto Mar 31, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

@jelbourn I've bumped this one down temporarily due to a compilation error coming in from core which is using a lower version of the typings.

crisbeto
crisbeto commented Mar 31, 2018
View reviewed changes
package.json
"core-js": "^2.4.1",
"rxjs": "6.0.0-beta.1",
"rxjs": "6.0.0-rc.0",
Copy link
Member Author

Choose a reason for hiding this comment

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

@jelbourn I'm not completely sure why Bazel is complaining about not being able to find rxjs. Do you have any ideas?

Copy link
Member

Choose a reason for hiding this comment

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

You need to bump the Bazel rules for nodejs and typescript in WORKSPACE. See here for the correct versions

You may also need to add @rxjs//operators to some BUILD files if Bazel still complains

Copy link
Member

Choose a reason for hiding this comment

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

One more thing, you also need to add "rxjs" to the list of node_modules in the top-level BUILD file. Then the only errors left should be the operators import.

Copy link
Member

Choose a reason for hiding this comment

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

After pulling and running locally, the following are the only BUILD files that need to be modified: cdk/a11y, cdk/overlay, and cdk/tree, and only the test sources need @rxjs//operators to build correctly

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for looking into it, but I'm not sure that's the case. I tried adding it to all of them and it ended up erroring on the cdk/portal.

Copy link
Member

Choose a reason for hiding this comment

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

You never added rxjs to the top-level BUILD node_modules

Copy link
Member

Choose a reason for hiding this comment

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

See here for reference

Copy link
Member Author

Choose a reason for hiding this comment

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

Looks like that did it, although it still feels weird to add rxjs/operators when it was complaining about rxjs/index.

Copy link
Member

Choose a reason for hiding this comment

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

The rxjs/index issue was resolved by adding the rxjs to top-level BUILD and updating the bazel rules. The operators are to reflect the latest changes in structure for RxJS RC

@crisbeto crisbeto force-pushed the ng-6.0.0-rc.1 branch 2 times, most recently from caf7113 to ea008c5 Compare March 31, 2018 17:10
@crisbeto crisbeto force-pushed the ng-6.0.0-rc.1 branch 3 times, most recently from 2140ff9 to 732b858 Compare March 31, 2018 17:29
@crisbeto crisbeto force-pushed the ng-6.0.0-rc.1 branch 2 times, most recently from 4ca5f83 to 1511a32 Compare March 31, 2018 17:40
josephperrott
josephperrott approved these changes Apr 2, 2018
View reviewed changes
Copy link
Member

@josephperrott josephperrott left a comment

Choose a reason for hiding this comment

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

LGTM, after the one comment.

WORKSPACE Outdated
url = "https://github.com/bazelbuild/rules_typescript/archive/0.11.1.zip",
strip_prefix = "rules_typescript-0.11.1",
sha256 = "7406bea7954e1c906f075115dfa176551a881119f6820b126ea1eacb09f34a1a",
name = "build_bazel_rules_typescript",
Copy link
Member

Choose a reason for hiding this comment

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

remove extra indenting from parameters.

The rest of the file only uses 2 spaces

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

@josephperrott josephperrott added pr: lgtm action: merge The PR is ready for merge by the caretaker target: patch This PR is targeted for the next patch release and removed pr: needs review labels Apr 2, 2018
jelbourn
jelbourn reviewed Apr 2, 2018
View reviewed changes
BUILD.bazel
@@ -26,6 +26,7 @@ filegroup(
"protobufjs",
"protractor",
"reflect-metadata",
"rxjs",
Copy link
Member

Choose a reason for hiding this comment

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

rxjs is actually not supposed to be here because it has its own bazel rules. Can you share the error you were getting?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't have the exact error, but it was something along the lines of "Can't find module rxjs/index".

Copy link
Member

Choose a reason for hiding this comment

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

Chatted w/ Alex, this seems like an issue w/ the ngsummary files being generated, we're figuring out the right way for core to fix this

@jelbourn jelbourn removed pr: lgtm action: merge The PR is ready for merge by the caretaker labels Apr 2, 2018
@CaerusKaru CaerusKaru mentioned this pull request Apr 2, 2018
Merged
@crisbeto
build: bump to angular 6.0.0-rc.1 and rxjs 6.0.0-rc.0
3fb4b3f 
Bumps to the latest Angular and RxJS RCs and fixes a compilation error in the postinstall script.
jelbourn
jelbourn approved these changes Apr 3, 2018
View reviewed changes
Copy link
Member

@jelbourn jelbourn left a comment

Choose a reason for hiding this comment

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

LGTM

@jelbourn jelbourn added action: merge The PR is ready for merge by the caretaker pr: merge safe labels Apr 3, 2018
@jelbourn jelbourn merged commit a9750c1 into angular:master Apr 3, 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 8, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@CaerusKaru CaerusKaru CaerusKaru left review comments

@jelbourn jelbourn jelbourn approved these changes

@josephperrott josephperrott josephperrott approved these changes

@andrewseguin andrewseguin Awaiting requested review from andrewseguin

@devversion devversion Awaiting requested review from devversion

@tinayuangao tinayuangao Awaiting requested review from tinayuangao

@mmalerba mmalerba Awaiting requested review from mmalerba

Assignees

@jelbourn jelbourn

Labels
action: merge The PR is ready for merge by the caretaker cla: yes PR author has agreed to Google's Contributor License Agreement target: patch This PR is targeted for the next patch release
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@crisbeto @CaerusKaru @jelbourn @josephperrott @googlebot