-
Notifications
You must be signed in to change notification settings - Fork 15
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
Add support for defer script tag #72
Conversation
@sir-dunxalot what do you think of this proposal? |
index.js
Outdated
} | ||
return '<script src="' + path + '"></script>\n'; | ||
|
||
return '<script' + scriptAttributes.join('') + ' src="' + path + '"></script>\n'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think about adding the space in the join()
? That seems less prone to future contribution error. For example:
scriptAttributes.push('async');
Then
return '<script' + scriptAttributes.join(' ') + ' src="' + path + '"></script>\n';
Hey @jeroenbell, I think this is OK! The only reason I am tentative is because I do not know if there will be any side effects of using Left a minor comment on the PR too - your call whether you want to address the comment. |
I've addressed your comment 🙂 We have done some internal testing with one of our applications using One thing I would like to add, is when using either |
Thanks @jeroenbell. I'm going to do a bit more reading but think this change is fine, especially since this is an 'opt-in' feature of sorts. Documentation and tests will need updating but that can be done after merge, before release. |
Released in 2.3.0. |
Similar to the
async
script tag, this adds an optionjs.useDefer
which adds<script defer ...>
. When enabling both options, most browsers will preferasync
, and fall back todefer
when support is limited.