microsoftgraph / microsoft-graph-toolkit

Authentication Providers and UI components for Microsoft Graph πŸ¦’
https://docs.microsoft.com/graph/toolkit/overview
Other
963 stars 309 forks source link

[BUG] Migrating to V3 cause mgt components to not work anymore in extension pages #2598

Closed stevebeauge closed 1 year ago

stevebeauge commented 1 year ago

Describe the bug I'm building a MS edge extension that require login.

I use the @microsoft/mgt-react package and especially the Login component to show a Sign in button.

The extension, built with the V2 of the MGT toolkit is working as expected : image

When I migrate to the V3, the MGT stopped to work: image

To Reproduce

I create a minimal repro: https://github.com/stevebeauge/repro-mgt-plasmo

git clone git clone "https://github.com/stevebeauge/repro-mgt-plasmo.git"
cd repro-mgt-plasmo
pnpm install
pnpm run dev

From Microsoft Edge:

  1. navigate to edge://extensions/
  2. enable Developer mode
  3. Load an Uncompressed extension
  4. Choose c:\path\to\repro-mgt-plasmo\build\chrome-mv3-dev folder
  5. From the extension menu, make the extension visible in the toolbar: image
  6. Click on the extension icon. It opens blank ❌
  7. Switch to v2 branch of the repo (or manually revert to @microsoft/mgt-react@2.11.2 in the package.json file
  8. pnpm install then pnpm run dev
  9. Open the extension popup again
  10. This time, the Sign in button is visible βœ…

Expected behavior Migrating to V3 should work

Additional information

In order to help diagnose the issue, you can open the page: extension://<<<ID of the extension>>>/popup.html. The ID is dependent on the path. It can been view in the extensions page of edge. In my case:

image

Which results in extension://mknmhhfclkaeokpkbihchgghfclaobdl/popup.html.

From this page, dev tools are available and we can see there's something in the DOM:

image

However, there's failures in the error stream:

image

Full error stack:

button.js:123  Uncaught TypeError: undefined is not iterable (cannot read property Symbol(Symbol.iterator))
    at Function.from (<anonymous>)
    at Button.connectedCallback (button.js:123:22)
    at Button.connectedCallback (index.js:21:14)
    at R.k (lit-html.ts:1415:4)
    at R.$ (lit-html.ts:1456:4)
    at R.g (lit-html.ts:1565:6)
    at R._$AI (lit-html.ts:1408:4)
    at D (lit-html.ts:2192:26)
    at MgtLogin.update (lit-element.ts:166:4)
    at MgtLogin.update (templatedComponent.ts:100:3)
connectedCallback @ button.js:123
connectedCallback @ index.js:21
k @ lit-html.ts:1415
$ @ lit-html.ts:1456
g @ lit-html.ts:1565
_$AI @ lit-html.ts:1408
D @ lit-html.ts:2192
update @ lit-element.ts:166
update @ templatedComponent.ts:100
performUpdate @ reactive-element.ts:1335
scheduleUpdate @ reactive-element.ts:1264
_$Ej @ reactive-element.ts:1239
await in _$Ej (async)
requestUpdate @ reactive-element.ts:1217
u @ reactive-element.ts:952
u @ reactive-element.ts:932
s @ lit-element.ts:131
MgtBaseComponent @ baseComponent.ts:58
MgtTemplatedComponent @ templatedComponent.ts:70
MgtLogin @ mgt-login.ts:115
createElement @ react-dom.development.js:9785
createInstance @ react-dom.development.js:10942
completeWork @ react-dom.development.js:22188
completeUnitOfWork @ react-dom.development.js:26598
performUnitOfWork @ react-dom.development.js:26568
workLoopSync @ react-dom.development.js:26468
renderRootSync @ react-dom.development.js:26435
performConcurrentWorkOnRoot @ react-dom.development.js:25740
workLoop @ scheduler.development.js:267
flushWork @ scheduler.development.js:241
performWorkUntilDeadline @ scheduler.development.js:534
runtime-41ccb8df9cb58c89.js:1 πŸ”΅ INFO   | [plasmo/parcel-runtime]: Connected to HMR server for D:\repro\plasmo+mgt\amuse-shy-jaguar\.plasmo\static\popup.tsx
runtime-d995119f79ca960d.js:1 πŸ”΅ INFO   | [plasmo/parcel-runtime]: Connected to HMR server for D:\repro\plasmo+mgt\amuse-shy-jaguar\node_modules\.pnpm\@plasmohq+parcel-transformer-manifest@0.17.7\node_modules\@plasmohq\parcel-transformer-manifest\runtime\plasmo-default-background.ts

When V2 is used, same devtools show:

image

Environment (please complete the following information):

ghost commented 1 year ago

Hello stevebeauge, thank you for opening an issue with us!

I have automatically added a "needs triage" label to help get things started. Our team will analyze and investigate the issue, and escalate it to the relevant team if possible. Other community members may also look into the issue and provide feedback πŸ™Œ

gavinbarron commented 1 year ago

@stevebeauge thanks so much for reporting this. To be honest with you using our components in a browser extension wasn't on my radar at all, so that's a really cool use case that we had no samples or test cases for.

I'm really sorry this is breaking your extension, thank for the repro, it illustrates the issue nicely. One more thing you could do to help us out here, do you know of a way to attach a debugger to the extension, say in VSCode, or really any mechanism of having a debugger open and attached before opening the UI pane of the extension?

Not being able to attach before the issue occurs makes investigating the issue a challenging one to investigate.

stevebeauge commented 1 year ago

@gavinbarron : I'm not used to open webpages with VSCode debugger open.

However, extensions are pages hosted on url like extension://mknmhhfclkaeokpkbihchgghfclaobdl/popup.html (id will be path dependent) and follow, I guess, the same rules than any other page in term of lifecycle.

That means you may be able to put breakpoints wherever you want and "refresh" the page with devtools opened.

Side note though, if you use any "devtools" extension (I had the issue with griffel devtools and react devtools), the devtool extension won't be able to access other extension page, unless a specific flag is set in the browser AND the devtools extension whitelist your extension (which is quite exclusive).

image

Just run pnpm run dev on a terminal to have the dev build version of the extension

gavinbarron commented 1 year ago

I worked out that if I inspected the popup, set a breakpoint and used the console to run location.reload(); I could debug the point that the exception is thrown from.

After a little testing and found that just using a bare fluent-button also has the incorrect behavior too, it's my belief that this defect is in the button coming from the @microsoft/fast-foundation package.

I'll submit an issue over there shortly.

gavinbarron commented 1 year ago

Reproduction with code changed to use a fluent-button only https://github.com/gavinbarron/repro-fluent-button-plasmo

gavinbarron commented 1 year ago

Hi @stevebeauge

I've done some more digging with some help from the folks over on the @microsoft/fast project, this looks like it's an issue in plasmo or parcel.

With v3 of MGT we moved to use the @fluentui/web-component library which is built on top of @microsoft/fast-foundation, so ultimately when we're using a button, we're importing this https://github.com/microsoft/fast/blob/archives/fast-element-1/packages/web-components/fast-foundation/src/button/index.ts As you can see there are two direct imports, one for the template and one for the logic.

If you open up the bundled popup.*.js file in an editor you can trace the dependency loading. Searching for the button.template.js import I find this:

var _buttonTemplateJs = require("./button.template.js");
parcelHelpers.exportAll(_buttonTemplateJs, exports);
var _buttonJs = require("./button.js");
parcelHelpers.exportAll(_buttonJs, exports);

},{"./button.template.js":false,"./button.js":"xTHA3","@parcel/transformer-js/src/esmodule-helpers.js":"boKlo"}],"xTHA3":[function(require,module,exports) {

So, to me this looks like the button template is being incorrectly tree shaken out.

louisgv commented 1 year ago

I think the main issue is that parcel follows ESM standard a bit too closely vs other framework that might be more forgiving with how they resolve ESM (i.e, webpack can be configured to mangle ESM from CJS package.json).

When looking at the package.json of https://www.npmjs.com/package/@fluentui/web-components?activeTab=code , the issue I'm seeing is that it doesn't have the module field or have an export field for module resolution, thus indicating that it is a bundled CJS module.

To be proper ESM, package.json should include export statement + module type, otherwise, parcel would ignore the ESM aspect of it and only grab the top-level file iirc..

stevebeauge commented 1 year ago

I narrowed even more the issue with a combination of parcel bundler + fast components : https://github.com/microsoft/fast/issues/6782

No react, no typescript, no fluent ui at all

stevebeauge commented 1 year ago

Regarding the previous discoveries, I found a workaround to make the MGT v3 happy with plasmo.

Basically, I created a popup/index.html, sibling to popup/index.tsx file.

I added explicitely @fluentui/web-components as a dependency, and, inside this html file:

<!DOCTYPE html>
<html>

<head>
    <title>__plasmo_static_index_title__</title>
    <meta charset="utf-8" />
</head>

<body>
    <script type="module" src="@fluentui/web-components/dist/web-components.js"></script>
</body>

</html>

This way, at runtime, it will use the statically linked library as is, instead of trying to bundle it. The fluentui will be able to find its required files.

This is not a proper solution, because it will break the tree shaking feature and import the whole library, but becasue the extension is loaded from local file system, it will be part of the extension setup.

gavinbarron commented 1 year ago

@stevebeauge I'm going to close this issue now as https://github.com/microsoft/fast/issues/6782 will address the root cause and you have a viable workaround in the mean time.