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.

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

Build update typescript to 3.4.x and rx js to 6.5.x #1795

Merged
merged 1 commit into from
May 6, 2019
Merged

Build update typescript to 3.4.x and rx js to 6.5.x #1795

merged 1 commit into from
May 6, 2019

Conversation

santoshyadavdev
Copy link
Sponsor Contributor

@santoshyadavdev santoshyadavdev commented Apr 23, 2019 •
edited

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

[ ] Bugfix
[x] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Documentation content changes
[ ] Other... Please describe:

What is the current behavior?

Closes #1788

What is the new behavior?

Does this PR introduce a breaking change?

[ ] Yes
[x] No

Other information

@santoshyadavdev
Copy link
Sponsor Contributor Author

WIP, please do not review

@timdeschryver timdeschryver changed the title Build update type script to 3.4.x and rx js to 6.5.x [WIP] Build update type script to 3.4.x and rx js to 6.5.x Apr 23, 2019
@brandonroberts brandonroberts added the WIP Not ready for review label Apr 23, 2019
@ngrxbot
Copy link
Collaborator

ngrxbot commented Apr 24, 2019

Preview docs changes for b553b4c at https://previews.ngrx.io/pr1795-b553b4c/

@santoshyadavdev
Copy link
Sponsor Contributor Author

Hi @brandonroberts ,
I have updated the package, stuck with one issue, external schematics is not generating the workspace, due to which test cases are getting failed.

Can you please guide i even tried to refer
https://github.com/angular/angular/blob/f8096d499324cf0961f092944bbaedd05364eea1/packages/elements/schematics/ng-add/index_spec.ts

@ngrxbot
Copy link
Collaborator

ngrxbot commented Apr 26, 2019

Preview docs changes for cf921f3 at https://previews.ngrx.io/pr1795-cf921f3/

@timdeschryver
Copy link
Member

I got this solved but it seems like I can't push to this branch 🤔

@ngrxbot
Copy link
Collaborator

ngrxbot commented Apr 27, 2019

Preview docs changes for 0115238 at https://previews.ngrx.io/pr1795-0115238/

@santoshyadavdev
Copy link
Sponsor Contributor Author

I got this solved but it seems like I can't push to this branch

Thanks Tim, it worked now.

@ngrxbot
Copy link
Collaborator

ngrxbot commented Apr 27, 2019

Preview docs changes for 12f4996 at https://previews.ngrx.io/pr1795-12f4996/

@santoshyadavdev
Copy link
Sponsor Contributor Author

I got this solved but it seems like I can't push to this branch

Hi @timdeschryver ,
This time it failed for bazel test step,no idea what wrong i am doing here, sorry for bothering you. :(

@ngrxbot
Copy link
Collaborator

ngrxbot commented Apr 28, 2019

Preview docs changes for f0249c4 at https://previews.ngrx.io/pr1795-f0249c4/

@ngrxbot
Copy link
Collaborator

ngrxbot commented Apr 28, 2019

Preview docs changes for 98f44a1 at https://previews.ngrx.io/pr1795-98f44a1/

@santoshyadavdev
Copy link
Sponsor Contributor Author

I got this solved but it seems like I can't push to this branch

Hi @timdeschryver ,
This time it failed for bazel test step,no idea what wrong i am doing here, sorry for bothering you. :(

I think it has something to do with bazel, i am referring to https://github.com/angular/angular/blob/master/WORKSPACE
Hope it's get resolved.

@timdeschryver
Copy link
Member

We probably have to update some bazel files 😅
I'll try to take a look at it tomorrow.

@ngrxbot
Copy link
Collaborator

ngrxbot commented Apr 28, 2019

Preview docs changes for c89f0ca at https://previews.ngrx.io/pr1795-c89f0ca/

@santoshyadavdev
Copy link
Sponsor Contributor Author

We probably have to update some bazel files
I'll try to take a look at it tomorrow.

Thanks, meanwhile am trying by referring angular repo.

timdeschryver
timdeschryver requested changes May 2, 2019
View reviewed changes
Copy link
Member

@timdeschryver timdeschryver left a comment

Choose a reason for hiding this comment

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

Add rjxs as a dep in schematics-core\build.bazel:

package(default_visibility = ["//visibility:public"])

load("//tools:defaults.bzl", "ts_library")

ts_library(
    name = "schematics-core",
    srcs = glob(
        [
            "**/*.ts",
        ],
    ),
    deps = [
        "@npm//@angular-devkit/core",
        "@npm//@angular-devkit/schematics",
        "@npm//typescript",
        "@npm//rxjs",
    ],
)

We should probably also rename this file to just BUILD to be consistent

modules/schematics-core/testing/create-workspace.ts Outdated
} from '@angular-devkit/schematics/testing';
import { concatMap } from 'rxjs/internal/operators/concatMap';
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
import { concatMap } from 'rxjs/internal/operators/concatMap';
import { concatMap } from 'rxjs/operators';

@santoshyadavdev
Copy link
Sponsor Contributor Author

santoshyadavdev commented May 2, 2019 •
edited

Not sure if this is the issue:
https://www.npmjs.com/package/ecstatic
this guy removed all the previous packages, but in that case how the build worked so far in the last 5 days.

jfhbrook/node-ecstatic#255

@ngrxbot
Copy link
Collaborator

ngrxbot commented May 3, 2019

Preview docs changes for cd3d644 at https://previews.ngrx.io/pr1795-cd3d644/

@ngrxbot
Copy link
Collaborator

ngrxbot commented May 3, 2019

Preview docs changes for 72c1682 at https://previews.ngrx.io/pr1795-72c1682/

@ngrxbot
Copy link
Collaborator

ngrxbot commented May 3, 2019

Preview docs changes for b10fcd4 at https://previews.ngrx.io/pr1795-b10fcd4/

@ngrxbot
Copy link
Collaborator

ngrxbot commented May 3, 2019

Preview docs changes for b44d0c9 at https://previews.ngrx.io/pr1795-b44d0c9/

timdeschryver
timdeschryver reviewed May 3, 2019
View reviewed changes
projects/ngrx.io/package.json
@@ -123,7 +123,7 @@
"hast-util-is-element": "^1.0.0",
"hast-util-to-string": "^1.0.0",
"html": "^1.0.0",
"http-server": "^0.9.0",
"http-server": "^0.11.1",
Copy link
Member

Choose a reason for hiding this comment

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

We had to update these because of a dependency removal - for more info see http-party/http-server#521

@timdeschryver timdeschryver mentioned this pull request May 3, 2019
Merged
3 tasks
@ngrxbot
Copy link
Collaborator

ngrxbot commented May 3, 2019

Preview docs changes for b44d0c9 at https://previews.ngrx.io/pr1795-b44d0c9/

timdeschryver
timdeschryver requested changes May 4, 2019
View reviewed changes
Copy link
Member

@timdeschryver timdeschryver left a comment

Choose a reason for hiding this comment

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

In default-pluralizer.spec we should move the pluralizer = TestBed.get(Pluralizer); assignment into the beforeEach block. This should solve the remaining failing tests.

@santoshyadavdev santoshyadavdev changed the title [WIP] Build update type script to 3.4.x and rx js to 6.5.x Build update type script to 3.4.x and rx js to 6.5.x May 4, 2019
@ngrxbot
Copy link
Collaborator

ngrxbot commented May 4, 2019

Preview docs changes for e03a2a8 at https://previews.ngrx.io/pr1795-e03a2a8/

@santoshyadavdev
Copy link
Sponsor Contributor Author

Open for review now.

@ngrxbot
Copy link
Collaborator

ngrxbot commented May 5, 2019

Preview docs changes for fedf74d at https://previews.ngrx.io/pr1795-fedf74d/

@santoshyadavdev santoshyadavdev mentioned this pull request May 5, 2019
Closed
3 tasks
@brandonroberts
Copy link
Member

I wonder why these tests needed to be converted to be async in the first place. Instead of using subscribes, add toPromise() to the runSchematicAsync and use async/await. Reference https://github.com/angular/angular-cli/blob/master/packages/schematics/angular/component/index_spec.ts

@brandonroberts
Copy link
Member

brandonroberts commented May 5, 2019 •
edited

This line needs to be updated to ^6.5.0 also https://github.com/ngrx/platform/blob/master/tools/defaults.bzl

@santoshyadavdev
Copy link
Sponsor Contributor Author

This line needs to be updated to ^6.5.0 also https://github.com/ngrx/platform/blob/master/tools/defaults.bzl

https://github.com/ngrx/platform/pull/1795/files#diff-653388f9d55f0ae80bba217809819c24R10
it is updated.

@santoshyadavdev
Copy link
Sponsor Contributor Author

I wonder why these tests needed to be converted to be async in the first place. Instead of using subscribes, add toPromise() to the runSchematicAsync and use async/await. Reference https://github.com/angular/angular-cli/blob/master/packages/schematics/angular/component/index_spec.ts

Hi @brandonroberts ,

After updating to the beta version, we started getting errors to convert external schematics to async.

@timdeschryver
Copy link
Member

timdeschryver commented May 5, 2019 •
edited

🤯 ugh, so much cleaner. My bad ... 😅
My search resulted at https://github.com/angular/angular/blob/master/packages/elements/schematics/ng-add/index_spec.ts#L44

@santoshyadavdev
Copy link
Sponsor Contributor Author

🤯 ugh, so much cleaner. My bad ...
I found https://github.com/angular/angular/blob/f8096d499324cf0961f092944bbaedd05364eea1/packages/elements/schematics/ng-add/index_spec.ts#L44 but didn't notice it was old.

Yes let me do this, so after this code we don't need to subscribe any more much cleaner code

@santoshyadavdev
Copy link
Sponsor Contributor Author

Hi @timdeschryver and @brandonroberts ,

I have changed the code, 6 test cases are failing will take a look into it tomorrow.

@ngrxbot
Copy link
Collaborator

ngrxbot commented May 5, 2019

Preview docs changes for ea72e05 at https://previews.ngrx.io/pr1795-ea72e05/

@santoshyadavdev
Copy link
Sponsor Contributor Author

Update failing test case for reducer only.

brandonroberts
brandonroberts requested changes May 5, 2019
View reviewed changes
modules/schematics-core/testing/create-workspace.ts Outdated
@@ -20,13 +22,6 @@ export const defaultAppOptions = {
skipTests: false,
};

const defaultModuleOptions = {
Copy link
Member

Choose a reason for hiding this comment

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

Leave this code as its an unrelated change

modules/schematics/src/cli.spec.ts Outdated
const content = tree.readContent(`${projectPath}/src/app/foo.ts`);

expect(content).toMatch(/export class Foo/);
schematicRunner
Copy link
Member

Choose a reason for hiding this comment

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

No subscribe, use async/await

modules/schematics/src/container/index.spec.ts Outdated
`${projectPath}/src/app/foo/foo.component.ts`
);
expect(content).not.toMatch(/import \* as fromStore/);
schematicRunner
Copy link
Member

Choose a reason for hiding this comment

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

No subscribe, use async/await

modules/schematics/src/container/index.spec.ts Outdated
expect(content).toMatch(/import \* as fromStore from '..\/reducers\/foo';/);
schematicRunner
.runSchematicAsync('container', options, appTree)
.subscribe(tree => {
Copy link
Member

Choose a reason for hiding this comment

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

No subscribe, use async/await

modules/schematics/src/container/index.spec.ts Outdated
expect(content).toMatch(/import \* as fromStore from '..\/reducers';/);
schematicRunner
.runSchematicAsync('container', options, appTree)
.subscribe(tree => {
Copy link
Member

Choose a reason for hiding this comment

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

No subscribe, use async/await

modules/schematics/src/container/index.spec.ts Outdated
`${projectPath}/src/app/foo/foo.component.ts`
);
expect(content).toMatch(/import\ {\ Store\ }\ from\ '@ngrx\/store';/);
schematicRunner
Copy link
Member

Choose a reason for hiding this comment

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

No subscribe, use async/await

modules/schematics/src/container/index.spec.ts Outdated
);
schematicRunner
.runSchematicAsync('container', options, appTree)
.subscribe(tree => {
Copy link
Member

Choose a reason for hiding this comment

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

No subscribe, use async/await

modules/schematics/src/container/index.spec.ts Outdated
);
schematicRunner
.runSchematicAsync('container', options, appTree)
.subscribe(tree => {
Copy link
Member

Choose a reason for hiding this comment

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

No subscribe, use async/await

modules/schematics/src/reducer/index.spec.ts Outdated
beforeEach(() => {
appTree = createReducers(createWorkspace(schematicRunner, appTree));
beforeEach(async () => {
appTree = await createWorkspace(schematicRunner, appTree);
Copy link
Member

Choose a reason for hiding this comment

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

Why was createReducers removed?

Copy link
Sponsor Contributor Author

Choose a reason for hiding this comment

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

It was by mistake and this is why my test cases were failing, thanks for pointing it out.

tools/defaults.bzl Outdated

DEFAULT_TSCONFIG = "//:tsconfig.json"
NG_VERSION = "^8.0.0-beta"
RXJS_VERSION = "^6.4.0"
RXJS_VERSION = "^6.5.1"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
RXJS_VERSION = "^6.5.1"
RXJS_VERSION = "^6.5.0"

Copy link
Member

Choose a reason for hiding this comment

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

why not 6.5.1?

@ngrxbot
Copy link
Collaborator

ngrxbot commented May 5, 2019

Preview docs changes for 64455d9 at https://previews.ngrx.io/pr1795-64455d9/

@ngrxbot
Copy link
Collaborator

ngrxbot commented May 5, 2019

Preview docs changes for 6c67e79 at https://previews.ngrx.io/pr1795-6c67e79/

@santoshyadavdev
Copy link
Sponsor Contributor Author

Hi @brandonroberts ,
Done with the changes. You can do the review now, all the test cases working now.

@ngrxbot
Copy link
Collaborator

ngrxbot commented May 5, 2019

Preview docs changes for 16442a0 at https://previews.ngrx.io/pr1795-16442a0/

@ngrxbot
Copy link
Collaborator

ngrxbot commented May 6, 2019

Preview docs changes for ad9e71a at https://previews.ngrx.io/pr1795-ad9e71a/

@alex-okrushko alex-okrushko mentioned this pull request May 6, 2019
Merged
3 tasks
brandonroberts
brandonroberts requested changes May 6, 2019
View reviewed changes
Copy link
Member

@brandonroberts brandonroberts left a comment

Choose a reason for hiding this comment

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

Make pinned framework versions consistent

package.json Outdated
"@angular/compiler-cli": "^8.0.0-beta.10",
"@angular/core": "^8.0.0-beta.10",
"@angular/forms": "^8.0.0-beta.10",
"@angular/animations": "^8.0.0-beta.13",
Copy link
Member

Choose a reason for hiding this comment

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

Don't mix the beta and rc versions. Pin all the framework versions to ^8.0.0-rc.0

Copy link
Sponsor Contributor Author

Choose a reason for hiding this comment

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

Done @brandonroberts I have updated the version, also in rc version, @angular/http dependency has been removed and the package is not available so removed the same.

@ngrxbot
Copy link
Collaborator

ngrxbot commented May 6, 2019

Preview docs changes for 04d547b at https://previews.ngrx.io/pr1795-04d547b/

@ngrxbot
Copy link
Collaborator

ngrxbot commented May 6, 2019

Preview docs changes for 607d231 at https://previews.ngrx.io/pr1795-607d231/

@brandonroberts
Copy link
Member

LGTM. Once Tim approves, we'll get it merged. Thanks for working through the changes!

timdeschryver
timdeschryver approved these changes May 6, 2019
View reviewed changes
Copy link
Member

@timdeschryver timdeschryver left a comment •
edited

Choose a reason for hiding this comment

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

👋 someone called me? 😁

@brandonroberts brandonroberts merged commit b49638a into ngrx:master May 6, 2019
@brandonroberts brandonroberts mentioned this pull request May 6, 2019
Closed
@alex-okrushko
Copy link
Member

@santoshyadav198613 Fantastic work! Thanks!

@santoshyadavdev
Copy link
Sponsor Contributor Author

@santoshyadav198613 Fantastic work! Thanks!

Thanks for appreciation@alex-okrushko mean a lot to me,

@santoshyadavdev
Copy link
Sponsor Contributor Author

LGTM. Once Tim approves, we'll get it merged. Thanks for working through the changes!

Thanks @brandonroberts for giving me the opportunity to work on it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alex-okrushko alex-okrushko alex-okrushko left review comments

@brandonroberts brandonroberts brandonroberts approved these changes

@timdeschryver timdeschryver timdeschryver approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Build: Update TypeScript to 3.4.x and RxJS to 6.5.x
5 participants
@santoshyadavdev @ngrxbot @timdeschryver @brandonroberts @alex-okrushko