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

Allow either 'defer', or 'async' attribute to be added to JavaScript loading in extensions #3078

Merged
merged 3 commits into from Mar 11, 2015
Merged

Allow either 'defer', or 'async' attribute to be added to JavaScript loading in extensions #3078

merged 3 commits into from Mar 11, 2015

Conversation

GwendolenLynch
Copy link
Contributor

UPDATED
The addJavaScript() function now:

  • Takes an $option array parameter (handles deprecated calls)
  • Allows extensions to defer, or asynchronously load their added JavaScript assets, e.g.:
$this->addJavascript('js/jquery.min.js', array('late' => true, 'priority' => 10, 'attrib' => 'async defer'));

Produces something similar to:

<script src="/js/jquery.min.js" async defer></script>

Changelog

  • Changed: The extension function now takes an option parameter array('late' => true, 'priority' => 10, 'attrib' => 'async defer')
  • Added: Ability for extensions to defer, or asynchronously load JavaScript

@cdowdy
Copy link
Contributor

cdowdy commented Mar 10, 2015

I'd make a slight change to this...

Instead of having just the singular versions async or defer I'd put asnyc and defer together to get

<script src="file.js" async defer></script>  

One could assume that if you're applying these to a script tag you know a little about web performance and not blocking the parser. So applying them both would be of great benefit. You'll get good web perf in new browsers and old browsers (back to IE 5 to ie 9) will use the defer attribute.

Defer is buggy in a lot of browsers. Async "just works ™" (in most use cases haha)

Here is why to use async defer
Ilya Grigorik Script Injected Async Scripts

@cdowdy
Copy link
Contributor

cdowdy commented Mar 10, 2015

wellllllll I'll just hold my horses... I noticed the "attrib" is just a string..... I take back my previous comment haha.. You could in theory pass both in a string since there doesn't seem to be any trimming or anything like that..

@GwendolenLynch
Copy link
Contributor Author

Thanks, @cdowdy.

The only thing is that async doesn't guarantee load order, so if I mark jQuery and Foundation both as async won't that mean that they may or may not work, just depends on… magic…?

@cdowdy
Copy link
Contributor

cdowdy commented Mar 10, 2015

so if I mark jQuery and Foundation both as async won't that mean that they may or may not work, just depends on… magic

yeah... same for defer too though. The script wont be loaded until after the document has been parsed.
Ideally since this is for extensions. if the extension author is including more than one script they would combine them into one. Then it wouldn't matter. The tricky part then comes to how to handle mutlple versions of jquery.. (which is another discussion haha)

@cdowdy
Copy link
Contributor

cdowdy commented Mar 10, 2015

Here is what I'm referring to when i mentioned defer above defer order buggy

If script 1 modifies the dom and script 2 is dependent on script 1 then script 2 can be executed before script 1 is finished and script 2 would have a dependency break (this is <= ie 9 and only mentioned because I believe bopp said bolt corp customers have to support ie 8?)

@GwendolenLynch
Copy link
Contributor Author

this is <= ie 9

Friends don't let friends use IE.

The tricky part then comes to how to handle mutlple versions of jquery

Probably not the greatest example, but close to the problem that I hit that I am trying to solve. My templates defer loading of jQuery and extensions that rely on it b0rk.

@cdowdy
Copy link
Contributor

cdowdy commented Mar 10, 2015

Friends don't let friends use IE.

wouldn't it be nice if everyone just all of a sudden started using ie 11 haha... few years ago it was "awww man ie 8 support" now its moved to ie 9 haha

Probably not the greatest example, but close to the problem that I hit that I am trying to solve. My templates defer loading of jQuery and extensions that rely on it b0rk.

yeah this is where an asset manager would be nice. be it assetic to combine and load all instances of a file to one css or js file or having code for extension template authors which says " if bolt jquery present load jq dependent js.. if not load minified and combined jq and jq dependent." but that isn't an easy task and my php skills are rather procedural programming instead of oop haha

@GwendolenLynch
Copy link
Contributor Author

yeah this is where an asset manager would be nice

Absolutely. Big job and someone needs to step up for it. I just wanted to solve problems in the short term with this one.

I'll have a better look today/tomorrow at those links and update this as required.

@bobdenotter
Copy link
Member

I'd like if we moved from:

public function addJavascript($filename, $late = false, $priority = 0, $attrib = false)

to:

public function addJavascript($filename, $options = array())

So we can do:

$this->addJavascript('foo.js', array('late' => true, 'attrib' => false));

@GwendolenLynch
Copy link
Contributor Author

How's this now @cdowdy & @bobdenotter

@cdowdy
Copy link
Contributor

cdowdy commented Mar 11, 2015

Looks good ! And like @bobdenotter said earlier having it as an array allows for anything that may be added in the future as well!

@bobdenotter
Copy link
Member

I like it! Good to go, if you ask me.

@bobdenotter
Copy link
Member

Merging in!

bobdenotter added a commit that referenced this pull request Mar 11, 2015
Allow either 'defer', or 'async' attribute to be added to JavaScript loading in extensions
@bobdenotter bobdenotter merged commit 2e7e789 into bolt:master Mar 11, 2015
@GwendolenLynch GwendolenLynch deleted the feature/javascript-defer-async branch March 11, 2015 18:28
GwendolenLynch referenced this pull request Mar 30, 2015
removed magnific popup css from footer, added async defer to app.js
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants