Add static typing #39

laurentdesmet · 2017-05-02T08:45:12Z

Add static typing using flow + newly generated docs.

coveralls · 2017-05-02T09:03:46Z

Coverage remained the same at 100.0% when pulling 951c71c on add-flow-types into c042d6a on master.

coveralls · 2017-05-02T13:04:56Z

Coverage remained the same at 100.0% when pulling 9130d70 on add-flow-types into c042d6a on master.

coveralls · 2017-05-02T13:40:24Z

Coverage remained the same at 100.0% when pulling 5db8cc0 on add-flow-types into c042d6a on master.

laurentdesmet · 2017-05-03T14:49:14Z

I noticed some of the types can be further narrowed down to a smaller subset, increasing the quality of the types so I would wait to merge this one right now. I'll further improve the types this week.

…nto add-flow-types

coveralls · 2017-05-08T12:37:00Z

Coverage decreased (-0.3%) to 99.694% when pulling 76f3d43 on add-flow-types into c042d6a on master.

coveralls · 2017-05-08T13:16:51Z

Coverage decreased (-2.4%) to 97.605% when pulling ae2e56f on add-flow-types into c042d6a on master.

coveralls · 2017-05-10T13:11:08Z

Coverage remained the same at 100.0% when pulling 0bd4e40 on add-flow-types into c042d6a on master.

laurentdesmet · 2017-05-10T13:33:59Z

@timdp This PR is ready to be reviewed again. I added static typing to all the src files, and also the tests. Added extra tests to cover the newly added methods and added a few typing fixes for our tests. For every property, I checked the VAST 3 and 4 xsd and usage in the iab-vast-parser to find out if it should be a maybe type or not.

Things that we maybe still could add in this PR:

Add a dedicated AbstractMethodError instead of a regular Error with a 'Abstract method' message.
I found out the AdBuffet is never exposed somewhere so I was wondering if we can leave this class out and move this behavior into the VAST class.
If we look at our types strictly, the required properties of each class should be passed through the constructor because now a newly constructed instance will have uninitialized properties where the types define them as non-nullable.

laurentdesmet · 2017-05-10T13:43:18Z

If this is merged, then I'll extend the codebase so the Wrapper instance has a reference to another VAST instance, which will be used in the async vast loader.

timdp · 2017-05-10T15:00:51Z

Add a dedicated AbstractMethodError instead of a regular Error with a 'Abstract method' message.

Definitely not against this. The main reason why we've been putting it off is because subclassing Error is messy, no matter how you do it.

I found out the AdBuffet is never exposed somewhere so I was wondering if we can leave this class out and move this behavior into the VAST class.

The spec defines it though. I kind of like to have it available as a concept, even if it's not immediately useful.

If we look at our types strictly, the required properties of each class should be passed through the constructor because now a newly constructed instance will have uninitialized properties where the types define them as non-nullable.

Yeah, I've been wondering what to do with the constructor vs. properties in general. I wouldn't be against passing a required options object into every constructor, as long as we also bump the major version, of course.

But maybe all of these things belong in separate PRs.

Before we merge this one, I just want to be absolutely sure that it's compatible with all our closed-source code (which I won't describe in detail because this discussion is public 😉).

laurentdesmet · 2017-05-11T09:10:24Z

Jup, all makes sense.

coveralls · 2017-06-19T20:46:08Z

Coverage remained the same at 100.0% when pulling d780a94 on add-flow-types into c042d6a on master.

coveralls · 2017-06-19T20:58:44Z

Coverage remained the same at 100.0% when pulling 0dd3441 on add-flow-types into c042d6a on master.

coveralls · 2017-06-20T09:27:33Z

Coverage remained the same at 100.0% when pulling c230216 on add-flow-types into c042d6a on master.

coveralls · 2017-06-20T12:40:36Z

Coverage remained the same at 100.0% when pulling cca72d1 on add-flow-types into c042d6a on master.

laurentdesmet · 2017-06-20T12:43:53Z

Ready for review again. Added support for shippable flow type definitions using flow-copy-source and added links between VAST and Wrappers which enable easy VAST tree walking.

timdp · 2017-07-31T13:29:09Z

gulpfile.babel.js


 gulp.task('watch', () => gulp.watch('src/**/*', ['build']))

-gulp.task('doc:clean', () => del('docs'))
+gulp.task('flow', async () => {


Is this async the only reason why you added stage-3 and the runtime transform, or is there another transitive dependency somewhere? If not, I'd just leave out the async because the end result will be the same. Also, maybe we want to drop Gulp altogether in favor of npm scripts?

timdp · 2017-07-31T13:30:50Z

README.md

 `docs` directory.

 Additionally, feel free to look at the source code. We recommend starting with
 `src/index.js`, which should export all the classes you need.

+## Typing
+
+All APIs are statically typed using (flow)[https://flow.org]. Type checking is automatically performed when running `npm test`. You can also run type checking only by using the command `npm run flow`. Static typing adds an additional layer of defense against errors and can also improve the development experience by enabling autocompletion. For (atom.io)[https://atom.io], we recommend (Flow-IDE)[https://atom.io/packages/flow-ide].


I'd leave out the last two sentences and keep the readme lean and mean. It's not our job to educate people on static typing or to impose an editor.

timdp · 2017-07-31T13:33:18Z

README.md

@@ -22,12 +22,16 @@ const vast = new VAST()

 Automatically generated API documentation can be found on
 [GitHub Pages](https://zentrick.github.io/iab-vast-model/). You can also
-open `docs/index.html` in a Web browser. Running `gulp doc` will (re)build the
+open `docs/index.html` in a Web browser. Running `npm run doc` will (re)build the


Probably not something to discuss under this PR, but I wonder why we have the docs on master rather than a separate gh-pages branch. There's quite a bit of static stuff here.

timdp · 2017-07-31T14:04:43Z

src/core/vast.js

+   * This will check if this VAST was wrapped and check if additional VAST
+   * wrappers should be loaded.
+   */
+  followAdditionalWrappers (): boolean {


Is there a reason why this isn't a getter?

Laurent De Smet added 5 commits April 27, 2017 19:47

add first bunch of flow types

abf48dc

add Type type

1e3829b

cleanup

e4cd3c9

some polishing

e687e38

change readme and integrate flow in test script

099656b

laurentdesmet requested a review from timdp May 2, 2017 08:45

Laurent De Smet added 2 commits May 2, 2017 10:57

add babel stage-3 support and add babel-runtime

bc6b84f

...

951c71c

Remove jsdoc leftovers; add missing rimraf dependency

9130d70

timdp added 2 commits May 2, 2017 15:37

[email protected]

4006f99

Update copyright year

5db8cc0

Laurent De Smet added 3 commits May 8, 2017 14:23

type improvements

a5a56c0

change docs

bfb07d7

Merge branch 'add-flow-types' of github.com:zentrick/iab-vast-model i…

76f3d43

…nto add-flow-types

improve methods

ae2e56f

Laurent De Smet added 2 commits May 10, 2017 15:06

add typing to tests

ec9d6e6

regenerate docs

0bd4e40

Laurent De Smet added 2 commits June 12, 2017 09:47

refactor ad buffet and ad pointers between Wrapper and its parent VAST

0e5cc3c

add additional tests

c436f47

regenerate docs

d780a94

update flow version

0dd3441

remove console.log

c230216

Laurent De Smet added 2 commits June 20, 2017 13:58

add flow-copy-source to build script

b1a0e4e

fix build script

cca72d1

This was referenced Jun 20, 2017

make iab-vast-parser compatible with new version of iab-vast-model zentrick/iab-vast-parser#48

Open

Async vast loading zentrick/iab-vast-loader#45

Open

timdp requested changes Jul 31, 2017

View reviewed changes

timdp reviewed Jul 31, 2017

View reviewed changes

Laurent De Smet added 2 commits September 5, 2017 13:40

Remove ads property

0a05025

WIP REMOVE COMMIT

092960c

timdp mentioned this pull request Feb 27, 2018

Add support for VAST 4 #46

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add static typing #39

Add static typing #39

laurentdesmet commented May 2, 2017

coveralls commented May 2, 2017

coveralls commented May 2, 2017

coveralls commented May 2, 2017

laurentdesmet commented May 3, 2017

coveralls commented May 8, 2017

coveralls commented May 8, 2017

coveralls commented May 10, 2017

laurentdesmet commented May 10, 2017

laurentdesmet commented May 10, 2017

timdp commented May 10, 2017

laurentdesmet commented May 11, 2017

coveralls commented Jun 19, 2017

coveralls commented Jun 19, 2017

coveralls commented Jun 20, 2017

coveralls commented Jun 20, 2017

laurentdesmet commented Jun 20, 2017

timdp Jul 31, 2017

timdp Jul 31, 2017

timdp Jul 31, 2017

timdp Jul 31, 2017

Add static typing #39

Are you sure you want to change the base?

Add static typing #39

Conversation

laurentdesmet commented May 2, 2017

coveralls commented May 2, 2017

coveralls commented May 2, 2017

coveralls commented May 2, 2017

laurentdesmet commented May 3, 2017

coveralls commented May 8, 2017

coveralls commented May 8, 2017

coveralls commented May 10, 2017

laurentdesmet commented May 10, 2017

laurentdesmet commented May 10, 2017

timdp commented May 10, 2017

laurentdesmet commented May 11, 2017

coveralls commented Jun 19, 2017

coveralls commented Jun 19, 2017

coveralls commented Jun 20, 2017

coveralls commented Jun 20, 2017

laurentdesmet commented Jun 20, 2017

timdp Jul 31, 2017

Choose a reason for hiding this comment

timdp Jul 31, 2017

Choose a reason for hiding this comment

timdp Jul 31, 2017

Choose a reason for hiding this comment

timdp Jul 31, 2017

Choose a reason for hiding this comment