Modularization #458

Rapha0511 · 2023-09-04T11:41:17Z

Description

The goal of the modularization is to let you choose precisly what action you want to achieve, and import only the code you need, without unrelated methods.

The main change is the introduction of a new Fetcher class, dedicated to fetching.

This allows:

The VAST Parser to be used for parsing only, without importing unnecessary methods related to fetching.
The VAST Client to be used for everything that needs to performe fetching AND parsing.

Here are the changes that have been made and that need to be taken in consideration when using the vast-client modularized.

New Private Fetcher Class:

This class includes:

A fetchVAST method used to fetch an XML document
All methods related to filtering the url before fetching.

All those methods were previously implemented in the VAST Parser

VAST Parser:

All the methods related to fetching have been moved to the new private class (Fetcher), dedicated to it.
The VAST Parser now contains only methods related to parsing.

Now, when using the parseVAST method from the VAST Parser, you will only be able to parse the first VAST encountered.
It is no longer possible to follow a wrappers chain using the parseVAST method ( if you want to do so, you should use the parseVAST method from the VAST Client).

Exemples:

When parsing directly an inline with the parseVAST method.

The inline vast will be parsed as before.

When parsing a Wrapper with the parseVAST method.

In this exemple, only the wrapper1.xml will be parsed.

No fetching will be performed, wrapper2.xml and inline.xml will be ignored because we use the VASTParser alone.

VAST Client:

The get method is no longer using the getAndParseVAST method from the VAST Parser that was use to fetch and parse VAST document.
Instead, it uses the fetchVAST method from the Fetcher and then parse the document with the parse method from VAST Parser.
All the methods related to filtering the url before fetching are now accessible using the VAST Client.
A parseVAST method has been added in order to directly parse an XML document and follow a wrappers chain if a wrapper is given.

VAST Tracker:

The VAST Tracker remains unchanged, you can still use it as before

Testing:

Every methods related to the new Fetcher class is tested in the fetcher.spec.js file, dedicated to it.
All tests related to Mocha have been migrated to Jest.
Mocha is no longer used in the project.

Bundling:

The IIFE format is no longer generated because it is now possible to import ES modules format by specifying the type="module" of your main file.
UMD format is no longer generated because it is obsolete, it is preferable to use ES modules

Additional informations:

To get the full benefit of those changes, projects using the vast-client must include three shaking.

Issue

The vast-client is divided in three components:

The VAST Client to fetch and parse VAST document.
The VAST Parser to directly parse VAST document.
The VAST Tracker to track the execution of an ad.

The VAST Parser in strongly dependant to the fetching, where is shouldn't.

When using the vast-client, you should be able to choose if you want to perform fetching and parsing or parsing only .

The VAST Parser should be used only if parsing is required.
The VAST Client must be used if fetching and parsing are required

All methods related to fetching should be separated to the VAST Parser since it is not part of the parsing.

Type

…t_parser

… class instead of getAndParseVAST from the vast_parser

…adding watch option when building the library

…iven xml

…inal inline object

…dle per module

ZacharieTFR

Huge work ! Thanks ✨
I will do a separate review for the documentation, and a second one with fresh eyes as it contains lots of changes ! 👌

ZacharieTFR · 2023-09-07T09:41:19Z

package.json

@@ -27,12 +33,11 @@
  "scripts": {
    "performance": "node performance/performance_test.js",
    "prebuild": "rm -rf dist_old && mkdir dist_old && cp -a dist/. dist_old/",
-    "build": "rollup -c",
+    "build": "rollup -c --watch",


I don't think we should have a --watch here, otherwise this will keep the process open and wait for any changes. Maybe having a separate command will be more convenient.

ZacharieTFR · 2023-09-07T09:49:20Z

rollup.config.js

-  const minifiedConfig = Object.assign({}, config);
-  minifiedConfig.output = Object.assign({}, config.output);
-  minifiedConfig.plugins = Object.assign([], config.plugins);
+// Node configuration


You can remove this comment and l.54 as they don't add additional informations

ZacharieTFR · 2023-09-07T09:52:08Z

rollup.config.js

-    outputFile.substr(0, extensionIndex) +
-    '.min' +
-    outputFile.substr(extensionIndex);
+function createNodeConfig(filePath, minifiedOutput, notMinifiedOutput) {


Why using a function instead of a const here?

ZacharieTFR · 2023-09-07T10:05:17Z

spec/fetcher.spec.js

+        fetcher.URLTemplateFilters = [(url) => url.replace('foo', 'bar')];
+        const expectedUrl = 'www.bar.foo';
+        fetcher
+          .fetchVAST(url, {}, 4, vastParser.emit.bind(vastParser))


Instead of using a real instance of vastParser you can mock it using a spy function as it's only used for the emit here and you don't want to test the whole chain but only how it was called.

ZacharieTFR · 2023-09-07T10:06:03Z

spec/fetcher.spec.js

+
+      it('should updates the estimated bitrate', () => {
+        jest.spyOn(Bitrate, 'updateEstimatedBitrate');
+        return fetcher


Suggested change

return fetcher

fetcher

ZacharieTFR · 2023-09-07T16:17:31Z

spec/vast_trackers.spec.js

+        spyTrack = jest.spyOn(vastTracker, 'trackURLs');
+        vastTracker.click();
+      });
+      it('should have sent  clickthrough event with clickThrough url', () => {


Suggested change

it('should have sent clickthrough event with clickThrough url', () => {

it('should have emit clickthrough event with clickThrough url', () => {

ZacharieTFR · 2023-09-07T16:18:28Z

spec/vast_trackers.spec.js

+    });
+  });
+
+  describe('#clickthroughs', () => {


Suggested change

describe('#clickthroughs', () => {

describe('#Linear', () => {

ZacharieTFR · 2023-09-07T16:18:48Z

spec/vast_trackers.spec.js

+    const fallbackClickThroughURL = 'http://example.com/fallback-clickthrough',
+      clickThroughURL = 'http://example.com/clickthrough';
+
+    describe('#VAST clichthrough with no fallback provided', () => {


Suggested change

describe('#VAST clichthrough with no fallback provided', () => {

describe('#VAST clickthrough with no fallback provided', () => {

ZacharieTFR · 2023-09-07T16:19:11Z

spec/vast_trackers.spec.js

+      beforeEach(() => {
+        const creative = createCreativeLinear();
+        creative.videoClickThroughURLTemplate = clickThroughURL;
+        vastTracker = new VASTTracker(vastClient, {}, creative);
+        spyEmit = jest.spyOn(vastTracker, 'emit');
+        vastTracker.click();
+      });
+      it('should have sent clickthrough event with VAST clickthrough url', () => {
+        expect(spyEmit).toHaveBeenCalledWith(
+          'clickthrough',
+          'http://example.com/clickthrough'
+        );
+      });


Can be merged together as there is no other assertion in the suite. Same below

ZacharieTFR · 2023-09-07T16:23:16Z

spec/fetcher.spec.js

Maybe we should regroup all spec files in the same folder WDYT ?

I think the folder hierarchy looks good like that as we have only specs in this forlder and their resources in samples

ZacharieTFR · 2023-09-08T12:00:24Z

spec/vast_parser.spec.js


-const xml = new DOMParser().parseFromString('<VAST></VAST>', 'text/xml');
+const xml = new DOMParser().parseFromString(


can getNodesFromXml from utils be used here ?

ZacharieTFR · 2023-09-08T12:04:18Z

spec/vast_parser.spec.js

+      //initParsingStatus always  will be called before parseVastXml
+      VastParser.initParsingStatus();


Why is it required ?

ZacharieTFR · 2023-09-08T12:05:55Z

spec/vast_parser.spec.js

+        .spyOn(VastParser, 'parse')
+        .mockImplementation(() => Promise.resolve([linearAd]));


Suggested change

.spyOn(VastParser, 'parse')

.mockImplementation(() => Promise.resolve([linearAd]));

.spyOn(VastParser, 'parse')

.mockReturnValue(Promise.resolve([linearAd]);

ZacharieTFR · 2023-09-08T12:19:42Z

spec/vast_parser.spec.js

+        res = response;
+        expect(res).toEqual({


You can only use response here

ZacharieTFR · 2023-09-08T12:21:34Z

spec/vast_parser.spec.js

+        expect(Bitrate.updateEstimatedBitrate).toHaveBeenCalledWith(
+          undefined,
+          undefined
+        );


This should never happen IMO

ZacharieTFR · 2023-09-08T12:49:02Z

spec/vast_parser.spec.js

      });
    });

    it('will successfully fetch the next wrapper url if it is provided', () => {
      const adWithWrapper = { ...ad, nextWrapperURL: wrapperBVastUrl };
-      jest.spyOn(VastParser, 'fetchVAST');
+
+      jest.spyOn(fetcher, 'fetchVAST').mockImplementation(() => {


Use mockReturnValue instead as you don't add logic

ZacharieTFR · 2023-09-08T12:55:15Z

README.md

@@ -47,9 +47,25 @@ vastClient.get('https://www.examplevast.com/vast.xml')

 In addition to fetching and parsing a VAST resource, **VASTClient** provides options to filter a sequence of calls based on count and time of execution, together with the possibility to track URLs using **VASTTracker**.

+If you need to directy parse a VAST XML and follow a wrappers chain, you can use the **VASTClient** just like so :


It's better to have it next to the get method IMO

Suggested change

If you need to directy parse a VAST XML and follow a wrappers chain, you can use the **VASTClient** just like so :

If you need to directly parse a VAST XML and also follow any wrappers chain, you can use the `parseVAST` method from the **VASTClient** :

ZacharieTFR · 2023-09-08T12:56:43Z

README.md

 ### VASTParser

 To directly parse a VAST XML you can use the **VASTParser**:
+The **VASTParser** will not proceed to any fetching, the final response will only contain the first VAST encountered.


Suggested change

The **VASTParser** will not proceed to any fetching, the final response will only contain the first VAST encountered.

The **VASTParser** will make no fetching, the final response will only contain the first VAST encountered.

ZacharieTFR · 2023-09-08T12:59:01Z

README.md


-You can add the script directly to your page and access the library's components through the `VAST` object.
+If you want to use this file, add it to your project and  make sure that your main file is in type module as below.


You should be as explicit as possible, because when you write add it to your project it doesn't explain how to actually add it.

ZacharieTFR · 2023-09-08T13:26:45Z

docs/api/vast-client.md

@@ -99,6 +101,74 @@ Instance of a class which implements the `Storage` interface. Should be set up o

 ## Public Methods 💚 <a name="methods"></a>

+### addURLTemplateFilter(filter)


Can you move thoses methods at the end ? We should keep the most important one at the top

…t macros values

…FileSize to exclude directory

ZacharieTFR · 2024-04-25T09:22:11Z

docs/api/vast-client.md

+*/
+```
+
+### removeURLTemplateFilter()


As it has been replaced with removeLastURLTemplateFilter you need to update it aswell

ZacharieTFR · 2024-04-25T09:25:18Z

src/fetcher.js

-   * @emits  VASTParser#VAST-resolving
-   * @emits  VASTParser#VAST-resolved


We should keep thoses informations, aswell as Returns a Promise which resolves,rejects according to the result of the request.

Upgraded fetching method, added error logging, and code Improvment

Rapha0511 · 2024-04-26T15:59:58Z

src/fetcher/fetcher.js

Switched back to .then instead of async/await to avoid "no-async-promise-executor" error.
According to ESLint documentation, it is not considered good practice to use an async function as the executor function within a new Promise.

see https://eslint.org/docs/latest/rules/no-async-promise-executor

If we want to use async/await, why not using async/await on fetchVAST? async fetchVast

Co-authored-by: Clarisse Damon <[email protected]>

…examples Adding vast client use case examples

Raphael Levin added 23 commits August 24, 2023 14:43

[tools] modification of the rollup configuration

c61c0b3

[parser] Removing fectchVAST and getAndParseVAST methods from the vas…

b2f40c4

…t_parser

adding new Fetcher class including the fetchVAST method

6a73db8

[client] changing get method to use fetchVAST method from the Fetcher…

2acf7da

… class instead of getAndParseVAST from the vast_parser

adding supported browsers list, modification of the entry point, and …

6ec4bce

…adding watch option when building the library

[parser] removing unnecessary comments

4d360bc

[client] add parseVAST method to be able to perform fetching from a g…

7e6f9f7

…iven xml

[test] tests for the modularization

06f729a

[test] moving mocha tests to jest

3d303d6

removing mocha command from package.json

f6229fb

[tools] Removing mocha

8ccc8d5

Removing unnecessary comment

3c23daf

[parser] checking Inline first to prevent VASTAdTagURI to be in the f…

f169bc5

…inal inline object

[client] rootURL should be set when the first VAST is fetch

f5a5575

[tools] updating rollup config since there is no need to have one bun…

06be203

…dle per module

[docs] WIP/modifying documentations for modularization

be2907f

[docs] WIP/ updating documentations

2c665d2

[docs] removing unnecessary documentation

41ae3a7

[docs] updating docs

33c34ef

[docs] updating documentations

ae5f7ce

[docs] updating README

5f7ade1

[docs] updating README

a26ab51

[docs] updating readme

fb4830c

ZacharieTFR requested changes Sep 7, 2023

View reviewed changes

ZacharieTFR reviewed Sep 8, 2023

View reviewed changes

Raphael Levin added 5 commits September 12, 2023 12:30

[test] updating vast version in tests files

9cd9f7b

[test] updating tests

5409cb6

[client] updating get method

33e0e79

[parser] updating fetchingCallback

1dbcc80

[tools] adding new command including --watch

0ca5915

Raphael Levin and others added 14 commits March 18, 2024 12:49

create folder for fetching related file

904e794

[tracker] fix comments related to vasttracker

49f2e8a

[utils] add two utils function to check browser environment and forma…

c51abd4

…t macros values

[client] fix import

b3b6822

[doc] add doc for new error tracking

074067e

[tool] fix rollup configuration to handle dynamic imports and fix get…

cf254d2

…FileSize to exclude directory

[parser] fix comments related to parseWrapper function

bbd2fc4

fix comments

fefb556

add a full example with a player using the vast-client

89a6b39

adding package.json

cb49bb5

update readme + VAST url

4d343b7

fix comments

0ce2697

removing safe operator for codesandbox compatibility

70dfd51

Update README.md

918c7d3

ZacharieTFR reviewed Apr 25, 2024

View reviewed changes

ZacharieTFR and others added 2 commits April 25, 2024 11:26

Merge pull request #467 from dailymotion/adding-error-logs

ba5bcd8

Upgraded fetching method, added error logging, and code Improvment

fix lint and comments

4bc57ca

Rapha0511 requested a review from ZacharieTFR April 26, 2024 15:43

Rapha0511 commented Apr 26, 2024

View reviewed changes

Raphael Levin and others added 9 commits April 26, 2024 18:26

fix comments

1754223

changing fetchVAST method to async

1381daf

fix lint

f3b596c

correct html , add h1, title and alt text for the image

955ab7a

fix typo

c5f03f0

Co-authored-by: Clarisse Damon <[email protected]>

play event refactorization

8adf88f

add package-lock and modify gitignore

feec407

Merge pull request #470 from dailymotion/adding-vast-client-use-case-…

fe7172c

…examples Adding vast client use case examples

fix lint in examples/utils.js

c1c8b42

Rapha0511 merged commit 37d5ac4 into master Jun 27, 2024
2 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Modularization #458

Modularization #458

Rapha0511 commented Sep 4, 2023

ZacharieTFR left a comment

ZacharieTFR Sep 7, 2023

ZacharieTFR Sep 7, 2023

ZacharieTFR Sep 7, 2023

ZacharieTFR Sep 7, 2023

ZacharieTFR Sep 7, 2023

ZacharieTFR Sep 7, 2023

ZacharieTFR Sep 7, 2023

ZacharieTFR Sep 7, 2023

ZacharieTFR Sep 7, 2023

ZacharieTFR Sep 7, 2023

Lotphy Sep 28, 2023

ZacharieTFR Sep 8, 2023

ZacharieTFR Sep 8, 2023

ZacharieTFR Sep 8, 2023

ZacharieTFR Sep 8, 2023

ZacharieTFR Sep 8, 2023

ZacharieTFR Sep 8, 2023

ZacharieTFR Sep 8, 2023

ZacharieTFR Sep 8, 2023

ZacharieTFR Sep 8, 2023

ZacharieTFR Sep 8, 2023

ZacharieTFR Apr 25, 2024

ZacharieTFR Apr 25, 2024

Rapha0511 Apr 26, 2024

GuillaumeBAECHLER Apr 26, 2024

	it('should have sent clickthrough event with clickThrough url', () => {
	it('should have emit clickthrough event with clickThrough url', () => {

	describe('#clickthroughs', () => {
	describe('#Linear', () => {

	describe('#VAST clichthrough with no fallback provided', () => {
	describe('#VAST clickthrough with no fallback provided', () => {


		const xml = new DOMParser().parseFromString('<VAST></VAST>', 'text/xml');
		const xml = new DOMParser().parseFromString(

		//initParsingStatus always will be called before parseVastXml
		VastParser.initParsingStatus();

		.spyOn(VastParser, 'parse')
		.mockImplementation(() => Promise.resolve([linearAd]));

		@@ -47,9 +47,25 @@ vastClient.get('https://www.examplevast.com/vast.xml')

		In addition to fetching and parsing a VAST resource, VASTClient provides options to filter a sequence of calls based on count and time of execution, together with the possibility to track URLs using VASTTracker.

		If you need to directy parse a VAST XML and follow a wrappers chain, you can use the VASTClient just like so :

	If you need to directy parse a VAST XML and follow a wrappers chain, you can use the VASTClient just like so :
	If you need to directly parse a VAST XML and also follow any wrappers chain, you can use the `parseVAST` method from the VASTClient :

	The VASTParser will not proceed to any fetching, the final response will only contain the first VAST encountered.
	The VASTParser will make no fetching, the final response will only contain the first VAST encountered.


		You can add the script directly to your page and access the library's components through the `VAST` object.
		If you want to use this file, add it to your project and make sure that your main file is in type module as below.

		@@ -99,6 +101,74 @@ Instance of a class which implements the `Storage` interface. Should be set up o

		## Public Methods 💚 <a name="methods"></a>

		### addURLTemplateFilter(filter)

		* @emits VASTParser#VAST-resolving
		* @emits VASTParser#VAST-resolved

Modularization #458

Modularization #458

Conversation

Rapha0511 commented Sep 4, 2023

Description

New Private Fetcher Class:

VAST Parser:

VAST Client:

VAST Tracker:

Testing:

Bundling:

Additional informations:

Issue

Type

ZacharieTFR left a 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment