-
Notifications
You must be signed in to change notification settings - Fork 389
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
refactor: convert app to latest Angular CLI format #641
Conversation
19462cc
to
d64fb9c
Compare
d64fb9c
to
c04b3ef
Compare
c04b3ef
to
ab9ba47
Compare
@@ -1,10 +1,16 @@ | |||
{ | |||
"$schema": "./node_modules/@angular/cli/lib/config/schema.json", |
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.
Are the changes for Angular CLI structure throughout this PR captured by a schematic? To what extent will these changes be required of other Angular CLI users? (Particularly ones who started a project in the early days like us)
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.
Most of the changes are not breaking or required changes. They mostly fall into the category of best practices. Over time, as these slight deviations from best practices build up, you can run into issues and/or unexpected breaking changes in future migrations.
Most projects that I've seen will try to align themselves with the best practices (i.e. output from a ng new
) from the latest version of the Angular CLI every couple of major releases. Currently, this repo seems to be aligned with the recommendations from Angular CLI v5.
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.
I guess what I really want to know here is how much of this change was for ivy and how much was was just event horizon for Angular CLI?
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.
I just went through and added comments for any Ivy-specific issues (only 2-3).
"e2e": "protractor", | ||
"e2e": "yarn build-themes && ng e2e", | ||
"build": "yarn build-themes && ng build", | ||
"build:highmem": "yarn build-themes && node --max_old_space_size=8192 ./node_modules/@angular/cli/bin/ng build", |
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.
Is this something commonly required, or is there something special about our app? I'm surprised that it's necessary given the relatively simple nature of our docs app
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.
It's tracked here: angular/angular-cli#13734. Based on this issue, it has become quite common and only minor progress has been made towards "solving it". In some cases, the CLI team has stated that increasing the available memory is expected.
I only ran into the issue when doing an AOT non-production build.
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.
I'm not sure if this would occur with Ivy as well since the Ivy build doesn't get that far into the build process, yet.
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.
It's also needed with Ivy.
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.
It looks like this is being addressed in PR angular/angular-cli#15777.
ab9ba47
to
09a5672
Compare
@@ -13,7 +13,8 @@ export class NavigationFocus implements OnInit { | |||
ngOnInit() { | |||
clearTimeout(lastTimeoutId); | |||
// 100ms timeout is used to allow the page to settle before moving focus for screen readers. | |||
lastTimeoutId = setTimeout(() => this.el.nativeElement.focus({preventScroll: true}), 100); | |||
lastTimeoutId = window.setTimeout(() => this.el.nativeElement.focus({preventScroll: true}), |
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.
For some reason the build always failed here because it thought this was referring to NodeJS's function which returns a NodeJS.Timeout
or NodeJS.Timer
. Adding window
was the widely accepted solution across StackOverflow and issue trackers.
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.
cc @devversion the nodejs typings shouldn't be making their way into the web compilation unit, but this is something we can deal with later
.pipe(map(params => params.get('theme')), filter(Boolean)) | ||
.subscribe(themeName => this.installTheme(themeName)); | ||
.pipe(map((params: ParamMap) => params.get('theme')), filter(Boolean)) | ||
.subscribe((themeName: string) => this.installTheme(themeName)); |
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.
This is needed for strictNullChecks
, but it wasn't detected previously. With the latest Angular/CLI/RxJS versions, it does become an error:
theme-picker.ts:75:51 - error TS2345: Argument of type 'unknown' is not assignable to parameter of type 'string'.
09a5672
to
2fff65f
Compare
@@ -9,10 +9,11 @@ import {unregisterServiceWorkers} from './unregister-service-workers'; | |||
// Unregister all installed service workers and force reload the page if there was | |||
// an old service worker from a previous version of the docs. | |||
unregisterServiceWorkers() | |||
.then(hadServiceWorker => hadServiceWorker && location.reload(true)); | |||
.then(hadServiceWorker => hadServiceWorker && location.reload()); |
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.
Passing true
for forceReload
is deprecated.
I just created an |
9a04091
to
4bad94e
Compare
afterEach(async () => { | ||
// Assert that there are no errors emitted from the browser | ||
const logs = await browser.manage().logs().get(logging.Type.BROWSER); | ||
expect(logs).not.toContain(jasmine.objectContaining({ |
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.
I'm not sure whether this will actually fail the test. A long time ago I tried doing something similar to detect leaking elements in unit tests and I couldn't get it to fail properly, although Jasmine could've changed the behavior since then. It might be better to either throw an error here or pull it out into a function that is called at the end of each test.
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.
This is taken directly from what the Angular CLI's e2e schematic generates for a new project.
If there is a little more concrete guidance here, I could open up an issue against Angular CLI.
fix some Ivy and TypeScript compilation and configuration issues add a `build:highmem` for use when the build fails due to memory usage update all dependencies that are supported remove unneeded polyfills now that differential loading is supported convert workspace to current format move and update tsconfigs to match current CLI format log errors on bootstrap enable some router quality of life options add a `Version` interface update StackBlitz - Copyright - major Angular version - dependencies fix build error for `setTimeout` - it was resolving to the NodeJS version rather than DOM version Work around angular/components#17264
4bad94e
to
858afc3
Compare
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
fix some Ivy and TypeScript compilation and configuration issues add a `build:highmem` for use when the build fails due to memory usage update all dependencies that are supported remove unneeded polyfills now that differential loading is supported convert workspace to current format move and update tsconfigs to match current CLI format log errors on bootstrap enable some router quality of life options add a `Version` interface update StackBlitz - Copyright - major Angular version - dependencies fix build error for `setTimeout` - it was resolving to the NodeJS version rather than DOM version Work around angular/components#17264
PR Checklist
Please check that your PR fulfills the following requirements:
What is the current behavior?
The Angular workspace and app project structure is not up to date with the latest recommendations from the Angular CLI.
Issue Number:
N/A
What is the new behavior?
Update to the latest recommendations from the Angular CLI in order to facilitate a future update to enable Ivy.
build:highmem
for use when the build fails due to memory usageVersion
interfacesetTimeout
Other information
This fixes some Ivy-related issues, but there are a few remaining fixes needed here and in Angular Material itself before the project will build with Ivy.