Skip to content

[Identity] Enable @azure/identity to build for the browser#4165

Merged
daviwil merged 12 commits intoAzure:masterfrom
daviwil:enable-identity-browser-build
Jul 8, 2019
Merged

[Identity] Enable @azure/identity to build for the browser#4165
daviwil merged 12 commits intoAzure:masterfrom
daviwil:enable-identity-browser-build

Conversation

@daviwil
Copy link
Contributor

@daviwil daviwil commented Jul 3, 2019

This change makes some tweaks to @azure/identity to enable it to build for and be used successfully in the browser. This is the first step toward adding user authentication in the browser for Preview 2.

The largest part of the change is breaking out all of IdentityClient's authenticate* methods into the individual Credential classes that use them so that they can be conditionally included in the browser build. I'm also stubbing out ClientCertificateCredential, ManagedIdentityCredential, and EnvironmentCredential entirely because they rely on the Node.js environment and modules.

I've tested that this works successfully using an example project that pulls @azure/identity in to a Webpack build that runs in the browser. I'll send a separate PR for that example project once it's ready.

@daviwil daviwil requested review from bterlson and schaabs July 3, 2019 05:26
@daviwil daviwil changed the title Enable @azure/identity to build for the browser [Identity] Enable @azure/identity to build for the browser Jul 3, 2019
@daviwil
Copy link
Contributor Author

daviwil commented Jul 3, 2019

OK, I think this is looking much better now. Question for @bterlson: should we be shimming Node-only classes to undefined like I'm doing here? I saw this being suggested in a storage PR and adopted the same strategy, but I'm not sure what the end result looks like to the user.

In the generated browser bundle I see the ClientCertificateCredential name being exported as undefined, so it seems like the user will be able to import it but will fail to be able to new it. Should there be a minimal shim that throws an exception with a helpful message?

@bterlson
Copy link
Member

bterlson commented Jul 3, 2019

In the storage case it worked because we were shimming functionality that would disappear completely in the browser build. Since this is shimming exported surface area, it's probably a good idea to give good errors if possible!

Copy link
Contributor

@sadasant sadasant left a comment

Choose a reason for hiding this comment

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

Looks good!! Can't wait to try it.

@daviwil daviwil force-pushed the enable-identity-browser-build branch from 2ce01be to bf83958 Compare July 5, 2019 18:21
@daviwil daviwil requested a review from bterlson July 5, 2019 18:21
@daviwil
Copy link
Contributor Author

daviwil commented Jul 5, 2019

OK, added class definitions for the non-supported credential types which throw error messages when browser use is attempted. Now I think this is ready for final review :)

import cjs from "rollup-plugin-commonjs";
import replace from "rollup-plugin-replace";
import { uglify } from "rollup-plugin-uglify";
import { terser } from "rollup-plugin-terser";
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this change required for the other sdks?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Probably so, I'll open a separate PR to make that change everywhere then we can decide whether to merge it.

@daviwil daviwil dismissed bterlson’s stale review July 8, 2019 20:26

I've addressed the comments in earlier changes

@daviwil
Copy link
Contributor Author

daviwil commented Jul 8, 2019

Going to merge this now to unblock other work. If anyone has any further comments I'll be happy to open another PR to address them. Thanks all!

@daviwil daviwil merged commit be8fccf into Azure:master Jul 8, 2019
@daviwil daviwil deleted the enable-identity-browser-build branch July 8, 2019 20:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants