[Service Bus] Add browser support #5128

ramya0820 · 2019-09-13T00:28:16Z

This PR addresses following changes as part of enabling browser support for use in production -

Fix rollup scripts
Update changelog
Add in copyrights
Update to use amqp-common preview.7 for reduced bundle size

ramya-rao-a · 2019-09-13T05:43:47Z

sdk/servicebus/service-bus/rollup.base.config.js

@@ -43,7 +43,7 @@ const ignoreKnownWarnings = (warning) => {
  console.error(`(!) ${warning.message}`);
 };

-export function nodeConfig({ test = false, production = false } = {}) {


@ramya0820 I am reluctant to take this change without first understanding the reason why the production flag was part of the function signature in the first place.

@bterlson I remember you had a reason to add the production flag here, but can't recall what it was. #2555 was the PR in which we moved away from using process.env.NODE_ENV to determine "production"

@ramya0820 I see that you had approved that PR. Do you recall the reason?

Discussed offline and addressed this along with other changes per #3373 description.

There was one temporary browser test failure, and we could add in better samples.
Other than these, we should be good to release once changes and contents in PR look okay.
Please take a look at the copyrights notice file.
@ramya-rao-a @bterlson

…to sb-browser-p1

ramya-rao-a

Please rename PR as this PR no longer about fixing rolling scripts and more of adding browser bundle in the package

ramya-rao-a · 2019-09-20T16:03:24Z

sdk/servicebus/service-bus/changelog.md

@@ -1,3 +1,8 @@
+# 2019-09-19 1.1.0


Suggested change

# 2019-09-19 1.1.0

# 2019-09-22 1.1.0

We mostly will not be releasing on a Friday

sdk/servicebus/service-bus/changelog.md

ramya-rao-a · 2019-09-20T16:05:48Z

sdk/servicebus/service-bus/src/serviceBusClient.ts

+      return ServiceBusClient.createFromTokenProvider(host, tokenProvider, options);
+    } else {
+      throw new Error("`createFromAadTokenCredentials` is not supported in browser.");
+    }


Reduce nesting by

if (!isNode) { // throw error here } // existing code that creates and uses aad token provider

Updated.

The reason I used isNode and not !isNode was keeping in view the rollup's "replace" plugin configuration that looks at the text " if (isNode) " and replaces it with " if (false) " in browser.
The changes seem equivalent though since we aren't enabling tree shaking. Maybe we should remove the replace plugin config? cc @bterlson

Addressed all other comments as well, please take a look

Yeah, better to use if (isNode) given the current rollup config. Short term fix might be to add the if (!isNode) pattern to the rollup config. Longer term fix is to have a platform.node.js and platform.browser.js that exports isNode = true and isNode = false respectively, that way either form will work fine with Rollup's tree shaker, and no replace plugin is required.

Filed: #5210

ramya-rao-a · 2019-09-20T16:07:24Z

sdk/servicebus/service-bus/src/serviceBusClient.ts

+      const tokenProvider = new AadTokenProvider(credentials);
+      return ServiceBusClient.createFromTokenProvider(host, tokenProvider, options);
+    } else {
+      throw new Error("`createFromAadTokenCredentials` is not supported in browser.");


The error message can be improved by alluding to the fact that AAD support is not yet present in browser mode and so createFromAadTokenCredentials method cannot be used to create ServiceBusClient

ramya-rao-a · 2019-09-20T16:12:34Z

Also, it would be great if we can release preview 7 of amqp-common first and use that version in this PR. The benefit of reduced bundle size that preview 7 of amp-common brings to the table will help the browser bundle.

bterlson

IMO remove the notices.txt change and we'll update these in one pass later (/cc @jonathandturner)

bterlson · 2019-09-21T00:02:29Z