Fix hooks

[iilei]

• Add tests for Hooks execution order and amount
• follow-up on Multiple link rel headers #140

[iilei]

I altered the integration test so it checks fragments serving fewer javascript files than permitted by maxAssetLink option and also added a test that checks execution of hooks.

[vigneshshanmugam]

• We should combine both id and fragmentId
• Use index only for script ordering
• Id/fragmentId for all the other cases - context, perf hooks

[vigneshshanmugam]

Check here https://github.com/zalando/tailor#fragment-attributes

[codecov-io]

Codecov Report

Merging #159 into master will increase coverage by 4.79%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #159      +/-   ##
==========================================
+ Coverage   91.77%   96.57%   +4.79%     
==========================================
  Files          13       13              
  Lines         523      525       +2     
  Branches       90       90              
==========================================
+ Hits          480      507      +27     
+ Misses         43       18      -25

Impacted Files	Coverage Δ
index.js	75% <ø> (ø)	⬆️
lib/fragment.js	100% <100%> (ø)	⬆️
lib/fetch-template.js	97.22% <0%> (+69.44%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1f1998d...4892792. Read the comment docs.

[vigneshshanmugam]

There is an interesting bug here, If a fragment( fragment-1) sends three scripts in Link header

Link: <module-1.js>; rel="fragment-script", <module-2.js>; rel="fragment-script", <module-3.js>; rel="fragment-script";

1. Module-1 exposes a default export function
2. Module-2 does not exposes any export function, Instead just executes some arbitrary stuff.
3. Same as module-1

In this scenario, The hooks except start for the fragment-1 will never run and all-done will not run as well.

This was not a problem before because we ignore if the fragment does not expose any default exports and fire all-done at last.

[iilei]

Good catch ... I haven't used require.js before and I wonder if there is some callback that would be able to handle the load event of arbitrary stuff.

[vigneshshanmugam]

Oops Sadly, this was broken before as well.. 'all-done' was never called if there was a script without a default export. I will try to find some hooks for require.js in the meanwhile.

[iilei]

I am also trying to find a way on another branch 'fix_hooks_wip'.
that approach would be less staight forward compared to the theoretical solution incorporating some kind of require callback, though.
let's see what works best in the end.

[vigneshshanmugam]

We need to handle three cases

1. if the script is not a AMD module, callback wont even get called
2. if script is AMD and does not have a init function(standalone)
3. if script is AMD and exposes a init fn (handled already - default)

We can handle the first one only if we can hook in to the require.js load function or resource loading logic.

[vigneshshanmugam]

I am starting to think, this logic should not be in tailor

1. Tailor should just fire onStart, onBefore, onAfter for all fragment scripts.
2. onDone should be called when all fragment's scripts are done initialising
3. Whoever wants to hook in to the logic should group them based on fragment Ids or whatever logic. We already handle the timingGroup feature in similar fashion https://github.com/zalando/tailor/blob/master/examples/fragment-performance/templates/index.html#L7

Thoughts? @iilei

[vigneshshanmugam]

I will create a new PR with your commits and some of my own.

[iilei]

I agree. That would be a good way of separating concerns and reducing the footprint of tailor. therefore 👍

[vigneshshanmugam]

This will fix the issue when onDone was not called for script without dependencies.

[vigneshshanmugam]

By keeping the id consistent, It ll become easy to measure the fragment perf. I will update the fragment-performance script to handle this case.

[vigneshshanmugam]

@iilei Its ready, have added some changes to your PR. Let me know your thoughts, We can merge once you review it.

[iilei]

@vigneshshanmugam great stuff 👍

I just added implicit testing the amount of hook execution by appending semicolons to the log lines.

[vigneshshanmugam]

@iilei I did manually remove it :) All good

[vigneshshanmugam]

Final rebase done. Merging it