Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Defer javascript and add feature Flag #2974

Merged
merged 3 commits into from Jan 28, 2020
Merged

Defer javascript and add feature Flag #2974

merged 3 commits into from Jan 28, 2020

Conversation

micgro42
Copy link
Collaborator

This builds upon the pull requests #2786 and #2958 by @Dr-Yukon and adds a feature flag for the defer attribute added to the jQuery and main-js requests in these pull requests. It adds only a single feature flag since deferring jQuery without deferring the main javascript request is likely to cause errors and confusion.

The feature flag defaults to "on" as this should be unproblematic except for a few plugins. Also, with this flag being on by default, it should see more usage and is more likely to uncover existing issues.

This feature flag should be removed once this feature is deemed safe.

Closes #2786
Closes #2958

Rainbow Spike and others added 3 commits January 26, 2020 18:52
GooglePage Speed Insights rank up 5-10 points
Otherwise, the browser render is waiting for loading and compiling big CDN script libraries
This adds a feature flag for the jQuery and main-js requests added in
 #2786 and #2958. This adds only a single feature flag since deferring
jQuery without deferring the main javascript request is likely to cause
errors and confusion.

The feature flag defaults to "on" as this should be unproblematic except
for a few plugins. Also, with this flag being on by default, it should
see more usage and is more likely to uncover existing issues.

This feature flag should be removed once this feature is deemed safe.
Copy link
Collaborator

@phy25 phy25 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! If we really introduce this concept of feature flag maybe we could iterate faster :-)

@Rainbow-Spike
Copy link
Contributor

[V]

@@ -230,6 +230,9 @@
$meta['search_fragment'] = array('multichoice','_choices' => array('exact', 'starts_with', 'ends_with', 'contains'),);
$meta['trustedproxy'] = array('regex');

$meta['_feature_flags'] = ['fieldset'];
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it make sense to add a new config section for this? I would have added it to the "Advanced" section. OTOH, if we do add a new section, it might make sense to rearrange some other settings as well?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I created that Feature Flag section to indicate that these Feature Flags are about to be dropped again soon. Maybe already before the next release, maybe only for the release after that.
To my understanding, Feature Flags are a specific pattern for rolling out features safely. https://en.wikipedia.org/wiki/Feature_toggle

It is not intended to permanently provide the ability to turn these features off, only until we are sure that we are not breaking too much.

If we want to make this a permanent toggle, then it should indeed be integrated into the "advanced" section. But I'm not sure I see the need for that?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense 👍

@Klap-in
Copy link
Collaborator

Klap-in commented May 10, 2020

Todo: Document feature flags, and this specific flag.

@Klap-in
Copy link
Collaborator

Klap-in commented Jun 2, 2020

Who could create this page: https://www.dokuwiki.org/config:defer_js ?
Please add purpose, and that this setting is a temporary feature flag to help bridge the period that plugins and templates need to implement it. Thanks!

@Klap-in Klap-in mentioned this pull request Jun 2, 2020
@splitbrain
Copy link
Collaborator

I started the page. Feel free to improve.

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

Successfully merging this pull request may close these issues.

None yet

5 participants