Async vast loading #45

laurentdesmet · 2017-06-20T12:48:09Z

Major rewrite of the loader to allow incrementally loading a VAST tree, with good error handling semantics and support for cancellation.

This PR depends on:

These PRs should be merged and published first (major update) before this one can be merged.

timdp · 2017-07-31T13:36:36Z

README.md

-Loads and parses IAB VAST tags, resolving wrapped tags along the way.
+Loads IAB VAST tag trees using a preorder depth first strategy. The package is statically typed using [Flow](https://flow.org). [Observable streams](http://npmjs.com/package/rxjs) are used to update the consumer in time with new VAST documents.
+
+This is a major rewrite from the [earlier version](https://github.com/zentrick/iab-vast-loader/tree/v0.8.0) of this package with the main benefit that it asynchronously fetches the complete VAST document tree. The previous version of this package used [promises](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Guide/Using_promises) and waited for the complete VAST tree to be fetched. One failing VAST document within the tree, made it fail completely. Cancellation semantics were also absent, by using Observables, we get them for free.


I'd rather not have the readme assume that people are familiar with previous versions, or at least not by default. We should move most of this to a section on upgrading.

timdp · 2017-07-31T13:41:23Z

README.md

+`10000`.
+- `retryCount`: The amount of times it will retry fetching a VAST document in case of failure. The default is `0`.
+- `credentials: Credentials`: Controls [CORS](https://en.wikipedia.org/wiki/Cross-origin_resource_sharing)
+behavior. You should pass an array of CredentialsType or functions that return a [CredentialsType value]((https://developer.mozilla.org/en-US/docs/Web/API/Request/credentials)), which is either `'omit'`, `'same-origin'` or `'include'`.


Not sure if typing (or the flow types below) is the best way to convey the API for credentials. Something like "either an array containing any of ['omit', 'same-origin', 'include'], or a function that maps the request URL to one of those values"?

Also, maybe we want to support a single string as well?

timdp · 2017-07-31T13:42:20Z

README.md

-`instanceof` doesn't work for you, you may want to inspect `error.$type`
-instead. This issue can occur if you load multiple versions of iab-vast-loader,
-each with their own `VASTLoaderError` class.
+  You can also pass multiple CORS strategies with the array. The implementation will race the different strategies in parallel, and will use the first request that succeeds. If none of the CORS strategies succeed, it will result in a `VAST_LOADING_FAILED` action. Notice that passing an empty array doesn't make sense, because it will make your request to fail always.


The "because ..." at the end is kind of weird.

timdp · 2017-07-31T13:43:31Z

README.md


-The maximum number of VAST documents to load within one chain. The default is
-10.
+The output of loadVast is a stream of VastLoadAction objects:


Maybe put identifiers in backticks.

timdp · 2017-07-31T13:44:36Z

README.md


-### `timeout`
+```js


Again not sure if I'd put flow types in the readme, but I'm not sure how else to do it. I don't want to scare people away just because they don't know about static typing though, because that shouldn't prevent them from using this library.

timdp · 2017-07-31T13:47:53Z

src/concat-eager.js

@@ -0,0 +1,76 @@
+// @flow


Maybe we want to publish this as a separate package?

timdp · 2017-07-31T13:49:31Z

src/rxjs-fx.js

+  Observable.defer(() => fetch(url, options))
+    .mergeMap(res => res.ok ? Observable.fromPromise(res.text()) : Observable.throw(new Error('Http Failed')))
+
+export const fx = {


What's fx stand for? Reactive fetch?

fx stands for effects because these are observables that introduce side effects when you subscribe to them.

timdp · 2017-07-31T13:52:13Z

src/rxjs-fx.js

+
+const http = (url: string, options?: RequestOptions) =>
+  Observable.defer(() => fetch(url, options))
+    .mergeMap(res => res.ok ? Observable.fromPromise(res.text()) : Observable.throw(new Error('Http Failed')))


Do we want to subclass Error for the HTTP-specific things? All fetch exceptions are just TypeErrors though, so meh.

timdp · 2017-07-31T13:59:04Z

test/lib/build-vast.js

+const fixturesPath = path.resolve(__dirname, '../fixtures')
+
+const buildVastVars = vastPath => {
+  const url = 'http://192.168.1.200:8080/' + vastPath


Can we make this dynamic using e.g. internal-ip?

timdp · 2017-07-31T14:00:51Z

test/lib/setup.js

@@ -4,10 +4,14 @@ import chaiAsPromised from 'chai-as-promised'
 import sinon from 'sinon'
 import sinonChai from 'sinon-chai'
 import dirtyChai from 'dirty-chai'
+// import {XMLHttpRequest} from 'xmlhttprequest'
+import XMLHttpRequest from 'xhr2'


This one isn't in package.json.

timdp · 2017-07-31T14:09:05Z

I'd still like to go over this together when you get back from holiday. 🙂

…oading to sequential vast loading

Laurent De Smet added 10 commits June 20, 2017 09:21

add flow definitions

6cf5198

implementation and initial tests

13cc834

add marble unit tests

919cece

Add lots of additional tests

49de389

add tests for rxjs-fx

c8dca54

remove package-lock.json file

2475bef

update readme

4e66ea6

update readme

c5f62c9

add flow-copy-source step to build process

fb70aa2

add flow command to scripts

d1e771c

laurentdesmet requested a review from timdp June 20, 2017 12:48

timdp requested changes Jul 31, 2017

View reviewed changes

Laurent De Smet added 3 commits September 4, 2017 16:53

allow strings for the credentials config and refactor parallel vast l…

390bc68

…oading to sequential vast loading

Update README and remove unused code

544207c

Update to make use of update VAST model structure

8e7f8cc

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Async vast loading #45

Async vast loading #45

laurentdesmet commented Jun 20, 2017 •

edited

Loading

timdp Jul 31, 2017

timdp Jul 31, 2017

timdp Jul 31, 2017

timdp Jul 31, 2017

timdp Jul 31, 2017

timdp Jul 31, 2017

timdp Jul 31, 2017

timdp Jul 31, 2017

laurentdesmet Aug 29, 2017

timdp Jul 31, 2017

timdp Jul 31, 2017

timdp Jul 31, 2017

timdp commented Jul 31, 2017


		### `timeout`
		```js

Async vast loading #45

Are you sure you want to change the base?

Async vast loading #45

Conversation

laurentdesmet commented Jun 20, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timdp commented Jul 31, 2017

laurentdesmet commented Jun 20, 2017 •

edited

Loading