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

perf(@ngtools/webpack): reduce rebuild performance by typechecking more #4258

Merged
merged 1 commit into from
Jan 30, 2017

Conversation

hansl
Copy link
Contributor

@hansl hansl commented Jan 28, 2017

We got the TypeScript performance down a lot by not type checking every files on rebuild, but this has an issue where typings can be wrong in files that depends on us. This PR alleviate the problem by type checking all files that depends on the changed file.

Note that even if this PR reduces performance, it's marginal by less than 5% (although it depends on the depth of the change in the dependency graph).

@hansl hansl force-pushed the typescript-check-dependencies branch 5 times, most recently from 20ae390 to 6b26281 Compare January 29, 2017 18:29
@@ -55,7 +55,7 @@
"ember-cli-string-utils": "^1.0.0",
"enhanced-resolve": "^2.3.0",
"exists-sync": "0.0.3",
"extract-text-webpack-plugin": "^2.0.0-rc.1",
"extract-text-webpack-plugin": "^2.0.0-beta.5",
Copy link
Contributor

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 changed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Merge conflict, I'm reverting this.

@@ -23,3 +23,29 @@ export interface ResolverPlugin extends Tapable {
join(relativePath: string, innerRequest: Request): Request;
}

export interface LoaderCallback {
Copy link
Contributor

Choose a reason for hiding this comment

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

Wouldn't it be better to use the existing Webpack typings?

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 @types/webpack package? It's super incomplete and fits more for using webpack than developing a plugin. I'm still looking for a good webpack typings from the webpack team itself.

if (!plugin.firstRun) {
this._module.reasons
.filter(reason => reason.module && reason.module instanceof NormalModule)
.forEach(reason => plugin.diagnose(reason.module.resource));
Copy link
Contributor

Choose a reason for hiding this comment

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

Will this also get reverse dependencies of reverse dependencies? e.g. index.ts re-exporting classes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I was doing that, and forgot to put it back, my bad. Thanks for catching it!

@@ -130,7 +130,7 @@
"@types/fs-extra": "^0.0.31",
"@types/glob": "^5.0.29",
"@types/jasmine": "^2.2.32",
"@types/lodash": "4.14.50",
"@types/lodash": "4.14.43",
Copy link

Choose a reason for hiding this comment

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

4.14.50 was pinned in #4260 recently merged

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. that was a merge conflict.

@hansl hansl force-pushed the typescript-check-dependencies branch from 6b26281 to f141cef Compare January 30, 2017 21:15
We got the TypeScript performance down a lot by not type checking every files on rebuild, but this has an issue where typings can be wrong in files that depends on us. This PR alleviate the problem by type checking all files that depends on the changed file.

Note that even if this PR reduces performance, we're still ~40% faster
than Beta.26.
@hansl hansl force-pushed the typescript-check-dependencies branch from f141cef to 761545e Compare January 30, 2017 21:24
@hansl hansl merged commit 29b134d into angular:master Jan 30, 2017
@hansl hansl deleted the typescript-check-dependencies branch January 30, 2017 22:42
MRHarrison pushed a commit to MRHarrison/angular-cli that referenced this pull request Feb 9, 2017
…re (angular#4258)

We got the TypeScript performance down a lot by not type checking every files on rebuild, but this has an issue where typings can be wrong in files that depends on us. This PR alleviate the problem by type checking all files that depends on the changed file.

Note that even if this PR reduces performance, we're still ~40% faster
than Beta.26.
@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 11, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants