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

feat(@angular-devkit/build-angular): Identify third-party sources in sourcemaps #23545

Merged
merged 1 commit into from
Jul 18, 2022
Merged

feat(@angular-devkit/build-angular): Identify third-party sources in sourcemaps #23545

merged 1 commit into from
Jul 18, 2022

Conversation

victorporof
Copy link
Contributor

@victorporof victorporof commented Jul 12, 2022 •
edited
Loading

This PR includes a webpack plugin which adds a field to source maps that identifies which sources are vendored or runtime-injected (aka third-party) sources. These will be consumed by Chrome DevTools to automatically ignore-list sources.

When vendor source map processing is enabled, this is interpreted as the developer intending to debug third-party code; in this case, the feature is disabled.

Signed-off-by: Victor Porof victorporof@chromium.org

@alan-agius4
Copy link
Collaborator

alan-agius4 commented Jul 12, 2022 •
edited
Loading

Do you know what is the build time impact of adding this plugin in a barebones angular project?

@victorporof
Copy link
Contributor Author

Do you know what is the build time impact of adding this plugin in a barebones angular project?

I don't. How can I benchmark this? I've also noticed there are some benchmark tests in the repo, would those catch any regressions?

alan-agius4
alan-agius4 reviewed Jul 12, 2022
View reviewed changes
packages/angular_devkit/build_angular/src/builders/browser/specs/vendor-source-map_spec.ts
@@ -50,3 +54,90 @@ describe('Browser Builder external source map', () => {
expect(hasTsSourcePaths).toBe(false, `vendor.js.map not should have '.ts' extentions`);
});
});

describe('Identifying third-party code in source maps', () => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

It's prefer if new tests were created using the new test harness. https://github.com/angular/angular-cli/tree/56320245a126481816a8ccfb0cc248a00be49561/packages/angular_devkit/build_angular/src/builders/browser/tests/behavior

But I guess we can migrate this as a whole eventually.

@alan-agius4
Copy link
Collaborator

I don't. How can I benchmark this? I've also noticed there are some benchmark tests in the repo, would those catch any regressions?

We actually don't have any benchmark tests. There is a bench-marker but you'd need to create and run the tests yourself.

Let me benchmark this myself and get back to you.

@victorporof victorporof changed the title feat(@angular/cli): Identify third-party sources in sourcemaps feat(@angular-devkit/build-angular): Identify third-party sources in sourcemaps Jul 12, 2022
@victorporof
Copy link
Contributor Author

Addressed comments.

@alan-agius4
Copy link
Collaborator

From my benchmark it appears that the plugin itself took about 500ms on a barebones project and ~2.8s on a real world application

Overall however, the build times didn't increase that much due to the async nature of the plugin.

@victorporof
Copy link
Contributor Author

Squashed.

@victorporof
Copy link
Contributor Author

victorporof commented Jul 12, 2022 •
edited
Loading

From my benchmark it appears that the plugin itself took about 500ms on a barebones project and ~2.8s on a real world application

Overall however, the build times didn't increase that much due to the async nature of the plugin.

We've found a way to reduce this from 2.8s to 200ms in the real world project by modifying the raw source maps directly instead of getting to them through the generated assets. Alan will push a commit.

Update: in the barebones project, with Alan's benchmark, this plugin takes 34ms.

alan-agius4
alan-agius4 requested changes Jul 12, 2022
View reviewed changes
Copy link
Collaborator

@alan-agius4 alan-agius4 left a comment

Choose a reason for hiding this comment

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

@victorporof, overall I am happy with this following the perf improvements that we did. Which in a real world app the processing took ~200ms were previously it took 2.9s

@clydin / @dgp1130 might want to take a look as well.

alan-agius4
alan-agius4 approved these changes Jul 12, 2022
View reviewed changes
Copy link
Collaborator

@alan-agius4 alan-agius4 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

@victorporof, overall I am happy with this following the perf improvements that we did. Which in a real world app the processing took ~200ms were previously it took 2.9s

@clydin / @dgp1130 might want to take a look as well.

@angular-robot angular-robot bot requested a review from alan-agius4 July 12, 2022 13:37
@alan-agius4 alan-agius4 added the target: minor This PR is targeted for the next minor release label Jul 12, 2022
alan-agius4
alan-agius4 approved these changes Jul 12, 2022
View reviewed changes
Copy link
Collaborator

@alan-agius4 alan-agius4 left a comment

Choose a reason for hiding this comment

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

LGTM

dgp1130
dgp1130 reviewed Jul 12, 2022
View reviewed changes
packages/angular_devkit/build_angular/src/webpack/plugins/devtools-ignore-plugin.ts Outdated Show resolved Hide resolved
packages/angular_devkit/build_angular/src/webpack/plugins/devtools-ignore-plugin.ts Outdated Show resolved Hide resolved
packages/angular_devkit/build_angular/src/webpack/plugins/devtools-ignore-plugin.ts Outdated Show resolved Hide resolved
packages/angular_devkit/build_angular/src/webpack/configs/common.ts Outdated
@@ -190,6 +194,19 @@ export async function getCommonConfig(wco: WebpackConfigOptions): Promise<Config
include.push(/css$/);
}

if (!vendorSourceMap) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm still a bit torn about whether to enable this in the vendorSourceMap case. I agree that users are likely to debug third party sources when it is enabled. However, I see DevToolsIgnorePlugin as just adding extra information about what sources are considered first party vs third party. It's up to Chrome to use that information in a useful manner for the user. IIUC, 3P files and stack frames would be hidden by default, but would include an override in DevTools to show them if they become useful for debugging.

Based on that, I'm thinking we should enable this plugin all the time so the information is always present. DevTools can always choose to ignore it when users decide to debug third party sources. This would be the workflow for cases where users are debugging their own code and stepping over 3P sources until they are about to reproduce a bug, then disable "just my code", and finally step into 3P source and continue debugging. I think we expect users to control this within DevTools anyways to be able to change their minds within a single page load, so I don't think there's much value in complicating this story by removing information in vendor source map cases.

WDYT? /cc @alan-agius4 @clydin

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fwiw, I don't feel strongly about this at all and happy with either decision.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think what @dgp1130 is suggesting makes sense.

By the way do devtools currently support the ignore list? I am not seeing any differenced in the devtools with our without this change.

Copy link
Contributor Author

@victorporof victorporof Jul 13, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

@alan-agius4 We're in the process of building this feature. We expect to land the parsing of x_google_ignoreList and the automatic ignore-listing feature soon (within weeks).

Copy link
Collaborator

Choose a reason for hiding this comment

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

This probably depends on a bit on what the DevTools UI actually looks like and how easy it is to discover and flip "just my code" as needed. Let's remove this condition for now and always include this data. If we find users are getting confused by it, then I think we can either treat that as a UI bug in DevTools or revisit this decision so vendorSourceMaps can be used as a CLI-level opt-out.

@victorporof victorporof force-pushed the devtools-ignore branch 2 times, most recently from 73f3c76 to 49be4af Compare July 15, 2022 09:39
@victorporof
Copy link
Contributor Author

Now generating the ignore-list even when vendored source maps processing is enabled.

copybara-service bot pushed a commit to ChromeDevTools/devtools-frontend that referenced this pull request Jul 15, 2022
@victorporof
Parse x_google_ignoreList in source maps
cbd9bb0 
Chrome DevTools is introducing a source map extension to identify
third-party sources in source maps. The `x_google_ignoreList` field
refers to the `sources` array, and lists the indices of all the known
third-party sources in that source map.

This metadata is intended to be used for automatic ignore-listing.

The Angular CLI will introduce support for `x_google_ignoreList` in:
angular/angular-cli#23545

Bug: 1323199
Change-Id: I0a340be4742a17b0d2b96107e650dd4421d59649
Signed-off-by: Victor Porof <victorporof@chromium.org>
Reviewed-on: https://chromium-review.googlesource.com/c/devtools/devtools-frontend/+/3757656
Reviewed-by: Jaroslav Sevcik <jarin@chromium.org>
@victorporof
feat(@angular-devkit/build-angular): Identify third-party sources in …
7c74fd1 
…sourcemaps

This PR includes a webpack plugin which adds a field to source maps that identifies which sources are vendored or runtime-injected (aka third-party) sources. These will be consumed by Chrome DevTools to automatically ignore-list sources.

When vendor source map processing is enabled, this is interpreted as the developer intending to debug third-party code; in this case, the feature is disabled.

Signed-off-by: Victor Porof <victorporof@chromium.org>
dgp1130
dgp1130 approved these changes Jul 15, 2022
View reviewed changes
Copy link
Collaborator

@dgp1130 dgp1130 left a comment

Choose a reason for hiding this comment

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

Thanks! Looking forward to trying this out once it's implemented in DevTools!

@alan-agius4 alan-agius4 added the action: merge The PR is ready for merge by the caretaker label Jul 15, 2022
@clydin clydin added target: rc This PR is targeted for the next release-candidate target: minor This PR is targeted for the next minor release and removed target: minor This PR is targeted for the next minor release target: rc This PR is targeted for the next release-candidate labels Jul 18, 2022
@clydin clydin merged commit f3087dc into angular:main Jul 18, 2022
@victorporof victorporof deleted the devtools-ignore branch July 19, 2022 17:22
@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 Aug 19, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@bmeurer bmeurer bmeurer left review comments

@alan-agius4 alan-agius4 alan-agius4 approved these changes

@clydin clydin clydin left review comments

@dgp1130 dgp1130 dgp1130 approved these changes

Assignees
No one assigned
Labels
action: merge The PR is ready for merge by the caretaker target: minor This PR is targeted for the next minor release
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@victorporof @alan-agius4 @bmeurer @dgp1130 @clydin