Skip to content
This repository has been archived by the owner on Apr 22, 2022. It is now read-only.

Rocket loader js friendly #179

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Rocket loader js friendly #179

wants to merge 1 commit into from

Conversation

iqbalhasnan
Copy link

Rocket loader [1] is a general purpose asynchronous Javascript loader that is used by Cloudflare to speed up the page load. Rocket loader will replace some of the native javascript functions with their own optimised functions and that cause issues with divolte.js

Issues caused by rocket loader:

1 - Access DOM array with [index] instead of item(index)

In this case, the HTML DOM item function is not part of the Rocket Loader function wrappers, therefore we need to access the element with [index] instead of item(index)

Rocket loader site
after
Normal site
after

2 - Rocket loader evaluates document.currentScript to null.

This will fail in https://github.com/divolte/divolte-collector/blob/master/src/main/resources/divolte.js#L87-L108 , because

typeof null === object

[1] https://support.cloudflare.com/hc/en-us/articles/200168056-What-does-Rocket-Loader-do-

please advise

Rocket loader [1] is a general purpose asynchronous Javascript loader that is used by Cloudflare to speed up the page load. Rocket loader will replace some of the native javascript functions with their own optimised functions and that cause issues with divolte.js

Issues caused by rocket loader:

1 - Access DOM array with [index] instead of item(index)

In this case, the HTML DOM item function is not part of the Rocket Loader function wrappers, therefore we need to access the element with [index] instead of item(index)

2 - Rocket loader evaluates `document.currentScript` to null.

This will fail in https://github.com/divolte/divolte-collector/blob/master/src/main/resources/divolte.js#L87-L108 , because

`typeof null === object`

[1] https://support.cloudflare.com/hc/en-us/articles/200168056-What-does-Rocket-Loader-do-
@asnare
Copy link
Member

asnare commented Dec 8, 2017

Not handling document.currentScript being null looks like a bug, and fixing it looks harmless.

The DOM array situation is a little more complex: getElementsByTagName() returns a NodeList, not an Array, and on some older browsers NodeList doesn't support array indexing. This is why we use .item(). Not supporting it looks like a bug in CloudFlare's Rocket loader. Have you mentioned this to them?

This notwithstanding, is there an easy way to automate testing of CloudFlare's Rocket loader?
These sorts of things might be fixable, but I fear that without a way to add them to our test suite the chance of regressions in future releases is high.

One final question: does everything work under CloudFlare if you add data-cfasync='true' to the script tag loading Divolte in your page?

@iqbalhasnan
Copy link
Author

iqbalhasnan commented Dec 11, 2017

One final question: does everything work under CloudFlare if you add data-cfasync='true' to the script tag loading Divolte in your page?

it works if I load the script this way

<script data-cfasync='false' src='/divolte.js'></script>

but it will not work when I load the script dynamically.

var script = document.createElement('script');
script.async = 1;
script.src ='divolte.js';
script.setAttribute('data-cfasync', false);
document.body.appendChild(script);

It seems CloudFlare ignores data-cfasync attribute when I load js dynamically

@iqbalhasnan
Copy link
Author

Not supporting it looks like a bug in CloudFlare's Rocket loader. Have you mentioned this to them?

I haven't mentioned item(i) issue to CloudFlare yet, but I did raised about disabling rocket loaded for dynamically loaded js.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants