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

Excluding JS files from Defer JS #3233

Closed
GeekPress opened this issue Oct 22, 2020 · 3 comments · Fixed by #3311
Closed

Excluding JS files from Defer JS #3233

GeekPress opened this issue Oct 22, 2020 · 3 comments · Fixed by #3311
Assignees
Labels
i18n Issues related to translation module: file optimization needs: documentation Issues which need to create or update a documentation type: enhancement Improvements that slightly enhance existing functionality and are fast to implement
Milestone

Comments

@GeekPress
Copy link
Contributor

What are we proposing to do? 

WP Rocket will include a UI to exclude JavaScript files from being deferred. 

Why are we doing this? 

Excluding JavaScript files from defer currently needs a helper plugin and is not the most user-friendly experience. This update will make it easier for all of our users to configure WP Rocket faster. 

This is a feature that has been requested by our users many times as well. #2290

User expectations and experience. How will we do this feature?

This feature will be implemented after we remove the “Safe Mode for jQuery” option. The UI proposal is as follows:

Text area title: Excluded JavaScript Files

Text area description: Specify URLs of JavaScript files to be excluded from defer (one per line).

Placeholder: /wp-content/themes/some-theme/(.*).js

Technical Specs

  • Current filter rocket_exclude_defer_js continues to work. This will make sure our current helper plugin works and existing customers won’t be impacted. 

  • Exclusions can be made with keywords from the URL, just the domain part, wildcards, or the entire URL. Examples of how it is right now are mentioned in our doc

  • Files excluded from defer can also be excluded from JavaScript combination if JS combination is enabled. They can still be minified. 

Risks

Since this is only a UI enhancement, we shouldn’t see risks of any kind. The existing helper plugin and the filter will continue to work as before. 

If anything, this should make things easier for users and support to resolve issues related to defer JS.

Acceptance Criteria

  • The UI is as per specification.

  • JavaScript files are excluded from defer based on full file name, keyword, or wildcards. All cases mentioned in the current doc works.

  • When external domains are mentioned, they are deferred too. 

  • Files excluded from defer are not combined by JavaScript combination. However, they are still minified. 

  • The existing helper plugin works (using the filter rocket_exclude_defer_js).

Documentation and Translations

The following documents need to be updated: 

@GeekPress GeekPress added epics 🔥 For large tasks or features, broken into smaller issues. module: file optimization needs: documentation Issues which need to create or update a documentation needs: grooming type: new feature Indicates the issue is a request for new functionality labels Oct 22, 2020
@GeekPress
Copy link
Contributor Author

GeekPress commented Oct 22, 2020

@arunbasillal arunbasillal added the i18n Issues related to translation label Nov 6, 2020
@GeekPress GeekPress added type: enhancement Improvements that slightly enhance existing functionality and are fast to implement and removed epics 🔥 For large tasks or features, broken into smaller issues. type: new feature Indicates the issue is a request for new functionality labels Nov 6, 2020
@arunbasillal arunbasillal added this to the 3.8 milestone Nov 9, 2020
@Tabrisrp
Copy link
Contributor

Tabrisrp commented Nov 9, 2020

Text area description: Specify URLs of JavaScript files to be excluded from defer (one per line).

This doesn't match with the line below:

Exclusions can be made with keywords from the URL, just the domain part, wildcards, or the entire URL. Examples of how it is right now are mentioned in our doc.

The description should be updated to reflect what is really expected in the field.

Files excluded from defer can also be excluded from JavaScript combination if JS combination is enabled. They can still be minified.

It doesn't work that way currently. Is this supposed to be a new enhancement, or was it an oversight?

@arunbasillal
Copy link
Contributor

@Tabrisrp

Text area description: Specify URLs of JavaScript files to be excluded from defer (one per line).
This doesn't match with the line below:
Exclusions can be made with keywords from the URL, just the domain part, wildcards, or the entire URL. Examples of how it is right now are mentioned in our doc.
The description should be updated to reflect what is really expected in the field.

I thought about this and figured that using the word URLs was the simplest to be part of the UI.

  • It works for internal files.
  • It works for external files as well. They can specify the full URL and that will be excluded.
  • The link to the documentation can show further examples which can be useful for advanced users.

@GeekPress @webtrainingwheels What do you think of this reasoning? ^ Please refer to Remy's comment for context if needed - #3233 (comment)

Files excluded from defer can also be excluded from JavaScript combination if JS combination is enabled. They can still be minified.
It doesn't work that way currently. Is this supposed to be a new enhancement, or was it an oversight?

Shouldn't this be done for defer to actually work? This was not an oversight.
If we combine the file that is excluded from defer and if that file is combined, then it defeats the purpose, right? Our current documentation says, If Minify JS or Combine JS is also active, you must exclude the files from that option as well.

@Tabrisrp Tabrisrp self-assigned this Nov 16, 2020
@Tabrisrp Tabrisrp linked a pull request Nov 17, 2020 that will close this issue
Tabrisrp added a commit that referenced this issue Nov 18, 2020
* Closes #3283 Exclude defer JS: Admin (PR #3291)
* Closes #3284 Defer JS exclusion: Front (PR #3297)
* Prevent combination of excluded files from defer JS (PR #3309)
* fix sanitizing for external files
iCaspar pushed a commit that referenced this issue Nov 20, 2020
* Closes #3283 Exclude defer JS: Admin (PR #3291)
* Closes #3284 Defer JS exclusion: Front (PR #3297)
* Prevent combination of excluded files from defer JS (PR #3309)
* fix sanitizing for external files
@Tabrisrp Tabrisrp mentioned this issue Nov 26, 2020
14 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
i18n Issues related to translation module: file optimization needs: documentation Issues which need to create or update a documentation type: enhancement Improvements that slightly enhance existing functionality and are fast to implement
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants