Skip to content
This repository was archived by the owner on Dec 18, 2024. It is now read-only.

build: update CLI version to 1.7.2 #366

Closed
wants to merge 1 commit into from
Closed

build: update CLI version to 1.7.2 #366

wants to merge 1 commit into from

Conversation

@rafaelss95
Copy link
Contributor

@rafaelss95 rafaelss95 commented Jan 5, 2018 •
edited
Loading

Closes #195.

@rafaelss95
Copy link
Contributor Author

rafaelss95 commented Jan 5, 2018 •
edited
Loading

I guess README.md needs to be updated.

For development builds use the usual ng serve and ng build commands.

ng serve doesn't work unless you run build-themes before (reported in #227) or yarn start.

Note:

While e2e failure isn't related to this PR, the saucelabs:

ERROR [karma]: Error: node_modules/@types/node/index.d.ts(100,13): error TS2451: Cannot redeclare block-scoped variable 'module'.

... was caused by these changes and I'm trying to figure out what's causing this error.

Can you take a look, @jelbourn? Thanks.

jelbourn
jelbourn suggested changes Jan 11, 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.

@rafaelss95 can you break this up into smaller PRs? It's difficult to review so many unrelated changes at once.

@rafaelss95
Copy link
Contributor Author

rafaelss95 commented Jan 12, 2018 •
edited
Loading

@jelbourn In fact, there are no unrelated changes. The proposed changes are a fully update in @angular/cli, to be in parity with latest version.

Look at the PR #143, for example, the cli version had been updated from "@angular/cli": "^1.0.0-beta.30" to "@angular/cli": "^1.0.0"... but just the version was updated, completely ignoring the fact that it changed a lot from beta to the final version (file names, structure, etc).

So, what I did here was: I created a fresh new project and compared all the changes, and there are a lot of changes, as you could seen.

@jelbourn
Copy link
Member

jelbourn commented Jan 12, 2018

It's not really necessary to update tslint rules and the source code at the same time as the CLI version.

@rafaelss95
Copy link
Contributor Author

rafaelss95 commented Jan 13, 2018

Well, those tslint rules are the default in the latest @angular/cli. I can undo those changes if you want.

@jelbourn
Copy link
Member

jelbourn commented Jan 16, 2018

Yeah- we don't necessarily use the default tslint rules here, anyway; we'd really want the same rules as the main material2 repo (except for the custom rules, which I don't want to copy-paste).

@rafaelss95
Copy link
Contributor Author

rafaelss95 commented Jan 17, 2018 •
edited
Loading

No problem, I undid the changes in tslint file.

@jelbourn jelbourn requested a review from amcdnl January 19, 2018 02:04
@jelbourn
Copy link
Member

jelbourn commented Jan 19, 2018

@amcdnl could you do a review pass?

amcdnl
amcdnl approved these changes Feb 10, 2018
View reviewed changes
Copy link
Contributor

@amcdnl amcdnl left a comment

Choose a reason for hiding this comment

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

I think it looks good overall, couple of questions for the author.

src/app/shared/navigation-focus/navigation-focus.ts
clearTimeout(lastTimeoutId);
// 100ms timeout is used to allow the page to settle before moving focus for screen readers.
lastTimeoutId = setTimeout(() => this.el.nativeElement.focus(), 100);
lastTimeoutId = window.setTimeout(() => this.el.nativeElement.focus(), 100);
Copy link
Contributor

@amcdnl amcdnl Feb 10, 2018

Choose a reason for hiding this comment

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

Why the window here?

Copy link
Contributor Author

@rafaelss95 rafaelss95 Mar 7, 2018

Choose a reason for hiding this comment

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

Not sure, but I guess I got an error when tried to run a specific command.

src/app/index.ts
@@ -1,2 +1,2 @@
export * from './material-docs-app';
export * from './app-module';
export * from './app.module';
Copy link
Contributor

@amcdnl amcdnl Feb 10, 2018

Choose a reason for hiding this comment

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

@jelbourn prefers the app-module format.

Copy link
Contributor Author

@rafaelss95 rafaelss95 Mar 7, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Really? :( I'll change ASAP.

tsconfig.json Outdated
{
"compileOnSave": false,
"compilerOptions": {
"allowSyntheticDefaultImports": true,
Copy link
Contributor

@amcdnl amcdnl Feb 10, 2018

Choose a reason for hiding this comment

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

Why was this added?

@amcdnl
Copy link
Contributor

amcdnl commented Feb 10, 2018

@rafaelss95 - Can you rebase?

@rafaelss95 rafaelss95 changed the title build: update CLI version to 1.6.3 build: update CLI version to 1.7.2 Mar 7, 2018
@rafaelss95
Copy link
Contributor Author

rafaelss95 commented Mar 7, 2018 •
edited
Loading

@amcdnl I've rebased, however now there a lot of errors. Lint errors (which ones I fixed in this PR) and others coming from @angular/cdk/tree and MatBottomSheet (you can see in Travis).

@rafaelss95
Copy link
Contributor Author

rafaelss95 commented May 27, 2018

Done in #431.

@rafaelss95 rafaelss95 closed this May 27, 2018
@rafaelss95 rafaelss95 deleted the update-cli branch May 27, 2018 15:40
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

2 more reviewers

@jelbourn jelbourn jelbourn requested changes

@amcdnl amcdnl amcdnl approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

cla: yes

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@rafaelss95 @jelbourn @amcdnl @googlebot