OfficeDev / Office-Addin-Scripts

A set of scripts and packages that are consumed in Office add-ins projects.
MIT License
152 stars 93 forks source link

[patch] Fix when add loopback exemption failed will break the debug process #847

Closed ZYUN-MSFT closed 3 months ago

ZYUN-MSFT commented 3 months ago

Change Description:

This PR fix the issue when add loopback exemption failed, the debug start process will totally break.\ The behavior will be the same as when user type No for add loopback exempt question.

Detail in this issue 840

Change QA

  1. Do these changes impact command syntax of any of the packages? No.
  2. Do these changes impact documentation? Maybe No.

I search the docs and found the following articles:

And accroding to issue 840, I assume that this PR won't change the docs.\ + @Rick-Kirkham for verify.

Validation/testing performed:

  1. Disable all loopback with fiddler tools
  2. Code change, build and global link the package office-addin-dev-settings
  3. Create a project with npx --package yo --package generator-office -- yo office --skip-install
  4. In this project, run the npm i and npm link office-addin-dev-settings
  5. Run npm run start
  6. In loopback question, type Y
  7. See following: npm run start
ZYUN-MSFT commented 3 months ago

Hi @millerds ,

I push the new change in this PR, which may resolve your concern.


The result of devSettings.ensureLoopbackIsEnabled:

And for following logic:


In my new commit, I move the logic from office-addin-dev-settings to office-addin-debugging.\ For condition throw error, the logic will log the original error and warn that add-in load maybe not correctly.\ Of course the add exemption failed will be record telemetry.

new result


Please comments if you have any other concern. 😃

Rick-Kirkham commented 3 months ago

I don't think it has any implications for documentation.

ZYUN-MSFT commented 3 months ago

More thoughts on telemetry:


According to https://learn.microsoft.com/en-us/office/dev/add-ins/testing/sideload-office-add-ins-for-testing#manually-sideload-an-add-in-to-office-on-the-web the loopback exemption is necessary only for Legacy Edge (beacuse Legacey Edge is a totally UWP).

image

https://github.com/OfficeDev/Office-Addin-Scripts/issues/840 also support the above conclusion.


For Webview2, it doesn't need loopback exemption. But original debug logic not only break Webview2 User's experience but also report an unnesaccery telemetry.\ The exception telemetry of Error: Access Denied, run the command as an administrator bring by webview2 user actually means nothing to us.

There will be fewer and fewer legacy edge users in the future. So the information bring by the count of Error: Access Denied, run the command as an administrator would not carry out a meaningful analysis conclusion.

millerds commented 3 months ago

More thoughts on telemetry:

According to https://learn.microsoft.com/en-us/office/dev/add-ins/testing/sideload-office-add-ins-for-testing#manually-sideload-an-add-in-to-office-on-the-web the loopback exemption is necessary only for Legacy Edge (beacuse Legacey Edge is a totally UWP).

image

840 also support the above conclusion.

For Webview2, it doesn't need loopback exemption. But original debug logic not only break Webview2 User's experience but also report an unnesaccery telemetry. The exception telemetry of Error: Access Denied, run the command as an administrator bring by webview2 user actually means nothing to us.

There will be fewer and fewer legacy edge users in the future. So the information bring by the count of Error: Access Denied, run the command as an administrator would not carry out a meaningful analysis conclusion.

When we are running this command we don't know what webview the host will be using so that doesn't play a factor. Error coming from our tooling are always important because they tell us what users are facing. Documentation tells us what is expected . . . but it doesn't tell us what customers are experiencing (does it match what our documentation says).

But my main point on telemetry is not whether or not the loop bypass hit an error . . . but whether or not the add-in had trouble loading (which is outside the tooling . . . so I'm not asking for logging, but rather knowledge of some other logs). If we can correlate add-in loading issues (or lack there of) with the bypass errors then we can get a better picture of what kind and how much of an impact this change has and it would be for useful in determining whether we can remove this this automated check. I want to understand the big picture . . . not this one use case.

millerds commented 3 months ago

/azurepipelines run

azure-pipelines[bot] commented 3 months ago
Azure Pipelines successfully started running 1 pipeline(s).