Skip to content

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

Merged
hansl merged 1 commit into
angular:masterfrom
hansl:typescript-check-dependencies
Jan 30, 2017
Merged

perf(@ngtools/webpack): reduce rebuild performance by typechecking more#4258
hansl merged 1 commit into
angular:masterfrom
hansl:typescript-check-dependencies

Conversation

@hansl
Copy link
Copy Markdown
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
Comment thread package.json Outdated
"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
Copy Markdown
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
Copy Markdown
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.

join(relativePath: string, innerRequest: Request): Request;
}

export interface LoaderCallback {
Copy link
Copy Markdown
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
Copy Markdown
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.

Comment thread packages/@ngtools/webpack/src/loader.ts Outdated
if (!plugin.firstRun) {
this._module.reasons
.filter(reason => reason.module && reason.module instanceof NormalModule)
.forEach(reason => plugin.diagnose(reason.module.resource));
Copy link
Copy Markdown
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
Copy Markdown
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!

Comment thread package.json Outdated
"@types/glob": "^5.0.29",
"@types/jasmine": "^2.2.32",
"@types/lodash": "4.14.50",
"@types/lodash": "4.14.43",
Copy link
Copy Markdown

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
Copy Markdown
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
Copy Markdown

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