Update/universalize app

[dmsnell]

While attempting to import this library into Automattic/wp-calypso I ran into unexpected problems as a result of the node core imports. Calypso's webpack configs explicitly exclude the built-in polyfills with node: false as the setting. As a result this fails to build properly there and is unusable.

While this can be resolved in Calypso it's indicative that this library doesn't follow what has become a standard for handling these cases: either use libraries which run in both node and in the browser; or use the "browser" field in package.json to provide replacement modules for the browser context.

In this patch we're applying both techniques to make this work in more contexts. We've replaced the use of util.inherits with the inherits package and replaced the use of util.format with string template literals. Finally instead of importing a generic requests library we're including native versions of the request mechanism for node and for the browser and using the "browser" field to choose the proper one.

[beaucollins]

Looks good to me. TIL about .browser.js.

The new http-request files are using spaces instead of tabs so eslint is lighting those files up with warnings for me:

/Users/beau/code/node-simperium/src/simperium/http-request.browser.js
   2:1  warning  Expected indentation of 1 tab but found 4 spaces   indent
   3:1  warning  Expected indentation of 2 tabs but found 8 spaces  indent
   5:1  warning  Expected indentation of 2 tabs but found 8 spaces  indent
   6:1  warning  Expected indentation of 2 tabs but found 8 spaces  indent
   8:1  warning  Expected indentation of 2 tabs but found 8 spaces  indent
   9:1  warning  Expected indentation of 2 tabs but found 8 spaces  indent
  11:1  warning  Expected indentation of 2 tabs but found 8 spaces  indent
  12:1  warning  Expected indentation of 1 tab but found 4 spaces   indent
  13:2  warning  Newline required at end of file but not found      eol-last

/Users/beau/code/node-simperium/src/simperium/http-request.js
   4:78  warning  'https$requestOptions' is not defined               no-undef
   5:1   warning  Expected indentation of 1 tab but found 4 spaces    indent
   6:1   warning  Expected indentation of 2 tabs but found 8 spaces   indent
   7:1   warning  Expected indentation of 3 tabs but found 12 spaces  indent
   9:1   warning  Expected indentation of 3 tabs but found 12 spaces  indent
  10:1   warning  Expected indentation of 4 tabs but found 16 spaces  indent
  11:1   warning  Expected indentation of 3 tabs but found 12 spaces  indent
  13:1   warning  Expected indentation of 3 tabs but found 12 spaces  indent
  14:1   warning  Expected indentation of 4 tabs but found 16 spaces  indent
  15:1   warning  Expected indentation of 3 tabs but found 12 spaces  indent
  16:1   warning  Expected indentation of 2 tabs but found 8 spaces   indent
  18:1   warning  Expected indentation of 2 tabs but found 8 spaces   indent
  19:1   warning  Expected indentation of 3 tabs but found 12 spaces  indent
  20:1   warning  Expected indentation of 2 tabs but found 8 spaces   indent
  22:1   warning  Expected indentation of 2 tabs but found 8 spaces   indent
  23:1   warning  Expected indentation of 1 tab but found 4 spaces    indent```

[dmsnell]

TIL about .browser.js.

@beaucollins you might have to unlearn it 😉 - there's no magic there. it's the "browser" field in package.json which provides the override to module resolution for browserify and webpack. I think that's a common name pattern but entirely arbitrary.

thanks for the review!

[belcherj]

Tested with Simplenote and everything appears to be working fine. Happy to take another look when you get CI running.

[dmsnell]

@beaucollins I have updated by removing endpoint but leaving options since it involves both appKey and appId

[beaucollins]

Since the node APIs and browser APIs have different concerns and types, it seems foregoing the ball of options makes sense. It's also simpler to add types.

Another bonus, import url from 'url' is now a concern of the node version, browser doesn't need it since XMLHTTPRequest can handle URL strings.

The external API of class Auth doesn't change at all:

diff --git a/src/simperium/auth.js b/src/simperium/auth.js
index 3d3fbc8..aa7c9f7 100644
--- a/src/simperium/auth.js
+++ b/src/simperium/auth.js
@@ -1,6 +1,5 @@
 // @flow
 import events from 'events'
-import url from 'url'
 
 import request from './http-request';
 
@@ -74,18 +73,9 @@ export class Auth extends EventEmitter {
 		return this.request( 'create/', body );
 	}
 
-	getUrlOptions( path: string ) {
-		const { port, ...options } = url.parse( `${ baseUrl }/${ this.appId }/${ path }` );
-		return (({
-			... options,
-			port: port ? Number( port ) : undefined,
-			method: 'POST',
-			headers: {'X-Simperium-API-Key': this.appSecret }
-		}: any): URL & { method: string, headers: { [string]: string } });
-	}
-
 	request( endpoint: string, body: string ): Promise<User> {
-		return request( body, this.getUrlOptions( endpoint ) ).then( response => {
+		const url = `${ baseUrl }/${ this.appId }/${ endpoint }`;
+		return request( this.appSecret, 'POST', url, body ).then( response => {
 			try {
 				const user = fromJSON( response );
 				this.emit( 'authorize', user );
diff --git a/src/simperium/http-request.browser.js b/src/simperium/http-request.browser.js
index 90d6533..ee167d0 100644
--- a/src/simperium/http-request.browser.js
+++ b/src/simperium/http-request.browser.js
@@ -1,15 +1,17 @@
 // @flow
 export default function(
+	appKey: string,
+	method: string,
+	url: string,
 	body: string,
-	options: URL & { method: string, headers: { [ string ]: string } }
 ): Promise<string> {
 	return new Promise( ( resolve, reject ) => {
 		const xhr = new XMLHttpRequest();
 
-		xhr.open( options.method, options.href );
+		xhr.open( method, url );
 		xhr.setRequestHeader(
 			'X-Simperium-API-Key',
-			options.headers[ 'X-Simperium-API-Key' ]
+			appKey
 		);
 
 		xhr.onload = () => resolve( xhr.responseText );
diff --git a/src/simperium/http-request.js b/src/simperium/http-request.js
index c5b41c6..1caa850 100644
--- a/src/simperium/http-request.js
+++ b/src/simperium/http-request.js
@@ -1,12 +1,22 @@
 // @flow
 import https from 'https'
+import { parse } from 'url';
 
 export default function(
+	appKey: string,
+	method: string,
+	url: string,
 	body: string,
-	options: URL & { method: string, headers: { [string]: string } }
 ): Promise<string> {
 	return new Promise( ( resolve, reject ) => {
-		const req = https.request( (options: any), (res: https.IncomingMessage) => {
+		const { port, ...options } = parse( url );
+		const req = https.request( {
+			...options,
+			method,
+			port: port ? Number( port ) : undefined,
+			agent: undefined,
+			headers: { 'X-Simperium-API-Key': appKey }
+		}, ( res: https.IncomingMessage ) => {
 			let responseData = '';
 
 			res.on( 'data', data => {

[beaucollins]

If you're still keen on using an options object, I would recommend creating an explicit type to be used for both versions of request:

// request-options.js
export type RequestOptions = {
   apyKey: string,
   method: string,
   host: string,
   ...
}

// http-request.s and http-request.browser.js
import type { RequestOptions } from './request-options';

export default function request( body: string, options: RequestOptions ) 

Then deal with transforming RequestOptions into something each implementation can use internal to each implementation.

[dmsnell]

I have updated this and tested by npm linking it with simplenote-electron and also by running it manually in node from a node shell.

[belcherj]

why this change? You only use request.

[dmsnell]

it's because the tests mock https

[dmsnell]

or I thought it was…updating back as it seems to work fine with it

[belcherj]

Tested and works