Add support for Browsers & Web Sockets by bterlson · Pull Request #29 · Azure/amqp-common-js

bterlson · 2019-02-15T22:46:35Z

This PR implements support for web sockets. This is required to enable AMQP support in the browser and to enable proxying support for Node.js.

amqp-common now uses the template build process. While I wouldn't expect this to be distributed separately from its dependencies (for now), this build process ensures the library can be built for the browser by dependents.

This works by adding some options to ConnectionContextBase#create:

webSocket, a reference to a WebSocket constructor (e.g. WebSocket in the browser, or require('ws') in node. Proxy support is enabled by applying the proxy config to the WebSocket prior to passing it in to this API.
webSocketEndpointPath, the path component to append after the host and port to reach the websocket endpoint, e.g. "$servicebus/websocket" for service bus.

Unfortunately, Rhea isn't super great in the browser - it depends on a number of very hefty node shims resulting in a fairly large bundle (~500KB). The most straight-forward way to enable this short-term is rollup-plugin-node-builtins but unfortunately NPM audit warns about this module. Another approach is to manually map node modules to browser dependencies.

Left to do:

Use API-Extractor to verify the external shape of the library hasn't changed with the updated dependencies.
Run tests in browser.
Browser-specific tests for web sockets
Node-specific tests for web sockets
Either remove rollup-plugin-node-builtins or find a way to silence the audit warnings in CI.

Future work:

Service Bus PR updating to this version of amqp-common.
Add tests for proxies (unclear presently how to do so using our CI infra, /cc @mikeharder)

Related to:

package.json

AlexGhiondea · 2019-02-20T06:07:03Z

package.json

    "is-buffer": "^2.0.3",
    "jssha": "^2.3.1",
-    "ms-rest-azure": "^2.5.9",
+    "rhea": "^0.3.9",


I remember a comment about rhea being a peer dependency -- not sure if that is a relevant piece of information.

That was rhea-promise.

Amar once mentioned that we shouldnt take a dependency on rhea directly and that we should use rhea-promise instead. I dont recall the reason why though...

If the rhea version resolved in amqp-common doesnt match with that in rhea-promise, we might end up with 2 versions of rhea correct?

Wasnt this was the reason why rhea-promise is a peer dependency in amqp-common?
If so, then rhea should be a peer dependency as well.

lib/ConnectionContextBase.ts

lib/connectionConfig/connectionConfig.ts

ramya-rao-a · 2019-02-21T02:57:44Z

package.json

    "is-buffer": "^2.0.3",
    "jssha": "^2.3.1",
-    "ms-rest-azure": "^2.5.9",
+    "rhea": "^0.3.9",


That was rhea-promise.

Amar once mentioned that we shouldnt take a dependency on rhea directly and that we should use rhea-promise instead. I dont recall the reason why though...

ramya-rao-a · 2019-02-21T02:59:34Z

package.json

+    "build": "npm run tslint && npm run tsc && cross-env rollup -c",
    "test": "npm run build && npm run unit",
-    "unit": "nyc --reporter=lcov --reporter=text-lcov mocha -r ts-node/register -t 50000 ./tests/**/*.spec.ts",
+    "unit": "cross-env TS_NODE_COMPILER_OPTIONS=\"{\\\"module\\\":\\\"commonjs\\\"}\" nyc --reporter=lcov --reporter=text-lcov mocha -r ts-node/register -t 50000 ./tests/**/*.spec.ts",


We could skip using ts-node altogether for running tests like we did in the service bus project

Can tackle this in a separate PR if we like?

vaish1707 · 2019-03-11T09:36:18Z

@ramya-rao-a , @bterlson Does these changes also adds proxy for storage blob? We also connect to storage blob while establishing a connection to azure event hubs.
In addition to this, I also have these questions.
Currently I'm trying to reach azure event hub from our cloud foundry environment. All our outgoing Http calls for the application deployed in the Cloud foundry are done by supplying the proxy configuration(https://docs.run.pivotal.io/buildpacks/proxy-usage.html) to the rest client.
When it comes to AMQP which is azure event hubs, until your proxy changes gets merged for azure event hubs as well, what is other alternative which we can look for?

bterlson · 2019-03-11T16:48:10Z

@vaish1707 I believe storage should already support proxies as that service uses HTTP rather than AMQP. I'm not sure if there are docs for supporting this though. @XiaoningLiu can you help @vaish1707 find docs for supporting proxies in the storage client?

Regarding support for proxies without this change landing, I wish I had a better answer. You could use the rhea library and talk AMQP directly to service bus, perhaps, but I think that would be a lot of work.

vaish1707 · 2019-03-11T17:29:53Z

Thanks for the reply. @bterlson is there any tentative date on when amqp proxy support will be available for eventhubprocessorhost for nodejs client? Also while adding the http proxy to storage blob . Is it going to be to append to the storage blob end point?

bterlson · 2019-03-11T18:15:26Z

The audit failure in CI is fixed by upgrading NYC in #31.

lib/auth/iotSas.ts

lib/ConnectionContextBase.ts

package.json

bterlson · 2019-03-13T00:18:27Z

@vaish1707 sorry I can't commit to a date, I'm just not sure.

Proxy support for Storage should be a matter of replacing the underlying http agent with something like http-proxy-agent. You shouldn't need to change anything else other than how the client is configured initially.

ramya-rao-a · 2019-03-13T01:17:50Z

lib/auth/aad.ts

 import {
  ApplicationTokenCredentials, DeviceTokenCredentials, UserTokenCredentials, MSITokenCredentials
-} from "ms-rest-azure";
+} from "@azure/ms-rest-nodeauth";


@bterlson When you tested Service Bus by pointing the amqp-common dependency to your git branch, were you able to run any of the examples like interactiveLogin, servicePrincipalLogin or the test on auth without making any changes?

@ramya-rao-a I didn't try the samples. I'll report here with my findings, though I may need your help getting the AD side set up properly.

lib/ConnectionContextBase.ts

vaish1707 · 2019-03-14T04:12:48Z

@ramya-rao-a ,@bterlson until the proxy support is available for the eventprocessorhost . We tried an alternative solution to open up the firewall for the ports 5671 and 5672 over TCP . We are still facing issues evening after opening up those ports over TCP. Also for storage blob we have opened up 443 and 80 over TCP.Are we missing something here? Can you please help on this?

AlexGhiondea · 2019-03-14T05:26:44Z

@vaish1707 would you mind opening a separate issue to discuss the issues you are running into? You can file an issue in the https://github.com/azure/azure-sdk-for-js repo and tag us in it!

ramya-rao-a · 2019-03-14T16:59:49Z

@alexandrudima, https://github.com/Azure/azure-event-hubs-node/issues/210 was logged by @vaish1707

@vaish1707 Let's have this discussion in the original issue you logged, I'll try to loop in the Event Hubs service team to get their insight

The previous commits depended on rhea getting an updated package.json.

bterlson · 2019-03-15T22:02:51Z

Now rebased and ready to merge once tests pass. FWIW, the only diff between the current new-msrest commit and the non-rebased commit (b387a88) is the NYC change to package.json:

git diff new-msrest bterlson/new-msrest -- package.json
diff --git a/package.json b/package.json
index ea188c5..06ed70e 100644
--- a/package.json
+++ b/package.json
@@ -53,7 +53,7 @@
     "karma-mocha": "^1.3.0",
     "mocha": "^5.2.0",
     "mocha-junit-reporter": "^1.18.0",
-    "nyc": "^13.3.0",
+    "nyc": "^12.0.2",
     "rhea": "^0.3.9",
     "rhea-promise": "^0.1.13",
     "rimraf": "^2.6.2",

AlexGhiondea reviewed Feb 20, 2019

View reviewed changes

ramya-rao-a suggested changes Feb 21, 2019

View reviewed changes

bterlson force-pushed the new-msrest branch from 2c5ab12 to d3d33c5 Compare March 8, 2019 23:15

ramya-rao-a reviewed Mar 11, 2019

View reviewed changes

lib/auth/iotSas.ts Show resolved Hide resolved

ramya-rao-a reviewed Mar 12, 2019

View reviewed changes

lib/ConnectionContextBase.ts Show resolved Hide resolved

ramya-rao-a reviewed Mar 12, 2019

View reviewed changes

package.json Show resolved Hide resolved

ramya-rao-a reviewed Mar 13, 2019

View reviewed changes

ramya-rao-a reviewed Mar 14, 2019

View reviewed changes

lib/ConnectionContextBase.ts Show resolved Hide resolved

ramya-rao-a approved these changes Mar 14, 2019

View reviewed changes

bterlson mentioned this pull request Mar 15, 2019

[Service Bus] Update amqp-common, ms-rest-azure Azure/azure-sdk-for-js#1596

Merged

bterlson added 12 commits March 15, 2019 14:47

Remove ms-rest-azure, add @azure/ms-rest-nodeauth and browserauth

81b47d2

Add browser support

19bd8a3

Add rollup configs

9995861

Add browser and dist-esm to .gitignore

1d0be5a

Add options for passing WebSocket constructor and setting WS endpoint

61b73b0

Add comment with purpose of index.browser.ts stub

8f76557

Remove @azure/ms-rest-browserauth dependency as it isn't needed for now

a171997

Update typescript version

97ec8f6

Fix TS error with an any cast

c904d84

Make tests run properly

8159dcc

Fix circular reference

85fdba0

Fix audit warnings (except nyc)

0214698

bterlson added 18 commits March 15, 2019 14:50

Explicitly import buffer

adc9eb7

Add shim for process.env

a0a62bd

Don't explicitly import mocha (the test harness will take care of that)

a6a0c38

Fix browser build and audit warnings, implement browser tests via karma

15d6b29

Properly use configured WebSocket constructor

45b34aa

Add tests for passing web sockets to ConnectionContextBase.create

9f8554a

Use WebSocket by default in the browser

f98cd50

Version 1.0.0-preview.2

f21c38f

Move browser build deps to dependencies

9c605fc

Fix syntax error in package.json

aa95e9f

Clean up lib dependencies; DOM not required anymore

06f62d4

Move rhea and rhea-promise to dev dep and peer dep

35422c5

Move WebSocket types to public surface area as types are exported

94f9fa8

Move some shims to rollup config

156dbc3

The previous commits depended on rhea getting an updated package.json.

Add a normal process shim; nextTick is required

f3cb8a4

Check for window before checking for window.WebSocket

a986899

peerDeps are external in rollup config as well

d07cedb

Remove some unused devdeps that crept in during rebase

7044f04

bterlson force-pushed the new-msrest branch from b387a88 to 7044f04 Compare March 15, 2019 21:56

Update package-lock.json

c22d361

bterlson added 2 commits March 18, 2019 15:58

Implement offline check for browser, fix build

62fd1d9

Add json plugin and dns shim to rollup config

b781cea

AlexGhiondea merged commit 5b0e82c into Azure:master Mar 18, 2019

ramya-rao-a mentioned this pull request Mar 19, 2019

Skip rhea dependency #35

Merged

Conversation

bterlson commented Feb 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vaish1707 commented Mar 11, 2019

Uh oh!

bterlson commented Mar 11, 2019

Uh oh!

vaish1707 commented Mar 11, 2019

Uh oh!

bterlson commented Mar 11, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bterlson commented Mar 13, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

vaish1707 commented Mar 14, 2019

Uh oh!

AlexGhiondea commented Mar 14, 2019

Uh oh!

ramya-rao-a commented Mar 14, 2019

Uh oh!

bterlson commented Mar 15, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

bterlson commented Feb 15, 2019 •

edited

Loading