View Issue Details

IDProjectCategoryView StatusLast Update
0020081mantisbtjavascriptpublic2015-12-06 02:45
Reporterdregad Assigned Todregad  
PrioritynormalSeveritymajorReproducibilityalways
Status closedResolutionfixed 
Product Version1.3.0-beta.3 
Target Version1.3.0-rc.1Fixed in Version1.3.0-rc.1 
Summary0020081: Moving JS to bottom is causing 'ReferenceError: jQuery is not defined' errors
Description

Implementation of 0013285 is causing a regression. The Snippets plugins [1], which atrol and I heavily use for user support, no longer works.

The Javascript log shows the following errors:

<pre>
16:28:50.864 ReferenceError: jQuery is not defined plugin_file.php:19:154
16:28:50.865 ReferenceError: jQuery is not defined plugin_file.php:7:4
16:28:50.865 ReferenceError: jQuery is not defined plugin_file.php:5:0
</pre>

[1] https://github.com/mantisbt-plugins/snippets

Steps To Reproduce
  • Install Snippets plugin
  • Display any bug (view.php)
  • Check Add Note text area: Plugin is not operational.
TagsNo tags attached.

Relationships

related to 0013285 closedsyncguru Move script inclusions from HEAD to document footer 
related to 0020088 closeddregad slow hiding of project-selector, filters 
related to 0020084 closeddregad EVENT_LAYOUT_BODY_END should fire after loading JS libraries 

Activities

dregad

dregad

2015-09-07 10:47

developer   ~0051384

The problem lies with EVENT_LAYOUT_RESOURCES plugin event, which is triggered at the end of HTML head.

The plugin relies on jQuery, but its own scripts are loaded before the library, which causes the error.

I'm not sure what the best way to fix this would be. One approach could be to use a distinct event for plugins to load javascript resources (e.g. EVENT_LAYOUT_RESOURCES_JAVASCRIPT) after jQuery's been loaded in html_body_end().

dregad

dregad

2015-09-07 11:05

developer   ~0051386

Furthermore, the event reference is now in contradiction with what the core actually does:

https://mantisbt.org/docs/master/en-US/Developers_Guide/html-desktop/#dev.eventref.output.layout

EVENT_LAYOUT_RESOURCES (Output)

This event allows plugins to output HTML code from inside the &lt;head> tag, for use with CSS, Javascript, RSS, or any other similar resources. Note that **this event is signaled after all other CSS and Javascript resources are linked by MantisBT**.
syncguru

syncguru

2015-09-07 16:17

developer   ~0051390

EVENT_LAYOUT_RESOURCES is a valid event event and should be used by plugins to link CSS & JS files.

The problem here is that the plugin has a dependency on JQuery. The plugin made an assumption about the presence of JQuery and when it is going to be loaded in the page. That's is the problem.

There are few ways to fix this:

(1) Move base JS libraries to <head> - backward compatible
Bring all base libraries to the <head> so that assumption remain valid. In this case we only need to have JQuery link move to head (jquery-ui does not unless plugin includes an extension to jquery-ui component)

(2) EVENT_LAYOUT_BODY_END
As a matter of best practice, JS files should go just before </body>. Plugins need to follow that. In case of Snippets plugin, that's a one line change.
Plugin authors can still put JS in head after EVENT_LAYOUT_RESOURCES provided that their is no dependency on any preloaded library.

dregad

dregad

2015-09-07 17:56

developer   ~0051391

Thanks for looking into this.

The problem here is that the plugin has a dependency on JQuery.

Correct. In 1.2, jQuery was a plugin of its own, so dependency was easily managed by making sure the jQuery plugin was loaded first.

The plugin made an assumption about the presence of JQuery

Again, correct. With 1.3, since jQuery is part of core, plugins should be safe to assume that the library is loaded for them when the documentation tells them they can load their resources.

and when it is going to be loaded in the page. That's is the problem.

Keep in mind that plugins do not have so many options to load resources since they must rely on available events as provided by Core. As per documentation, EVENT_LAYOUT_RESOURCES was the right way to do things, until 0013285.

For the record, I am not saying that moving JS to the bottom was bad or wrong, or that the plugin should not adapt to the new method.

Now with regards to your proposed options:

(1) Move base JS libraries to <head> - backward compatible
[...] we only need to have JQuery link move to head

IMO, this option implies fully reverting 0013285. Doing things half-way (i.e. only moving jQuery back up) doesn't make sense, as sooner or later we'll face the same issue with another plugin which relies on jQueryUI.

(2) EVENT_LAYOUT_BODY_END

This event currently is not placed in the right location: it is triggered before jQuery is loaded. This implies either

  • moving the event further down, e.g. just before the closing </div>, or
  • creating a new event for the specific purpose of loading jQuery-based javascript resources, as I suggested earlier

After that, all plugins relying on jQuery or jQueryUI need to be adapted to use the new event.

As a matter of best practice, JS files should go just before </body>

I assume you're basing your recommendation on this Yahoo developers article [1].

I'd like to challenge that, and point out another argument which goes the other way, i.e. it's better to have a page that appears to load slightly more slowly, than a page that appears broken [2].

Based on the above, my personal choice would be option 1, i.e. move jQuery/jQueryUI back to the top.

[1] https://developer.yahoo.com/performance/rules.html
[2] http://demianlabs.com/lab/post/top-or-bottom-of-the-page-where-should-you-load-your-javascript/

vboctor

vboctor

2015-09-07 18:30

manager   ~0051394

Link to @syncguru's PR: https://github.com/mantisbt/mantisbt/pull/642

@dregad The article you referenced makes sense. This would have been a good discussion as part of implementing 0013285.

I suggest we start off with the implementation that @syncguru provided in his PR which should fix the plugins. If we notice issues coming in about app being broken and as we increase our dependency on Javascript, we can revert 0013285 and move the JS back to head.

dregad

dregad

2015-09-07 18:57

developer   ~0051396

This would have been a good discussion as part of implementing 0013285.

Indeed... I only just found it while researching a bit on this so-called "best practice", sorry ;)

the implementation that @syncguru provided in his PR which should fix the plugins

Actually it would not I'm afraid, at least not without altering the plugins' code to load javascript resources on EVENT_LAYOUT_BODY_END instead of EVENT_LAYOUT_RESOURCES.

I would therefore like us to complete the discussion here before changing plugins, as I'd rather avoid having to revert them after reverting 0013285 if it comes to that.

syncguru

syncguru

2015-09-07 19:06

developer   ~0051397

@dregad - the excerpt below is from document [2]

"Summarising, it really depends on what you’re making. If you’re still stuck in the “content” world then it’s maybe best to place your JavaScript at the bottom of the page. But if you are making web software you’re much better, placing JavaScript at the top, because functionality is the new king."

I totally agree with the author assessment. For heavy JS app, most of the code will live in the JS thus there is no point in putting JS at the bottom. If you think about modern Backbone or Angular apps, there is really nothing in the page except that links to JS & CSS files.

Mantis is largely a content app. The JS is very lightly used (if any in most pages). So putting the JS at the bottom for MantisBT aligns well with the author argument (and possibly yours)

vboctor

vboctor

2015-09-07 20:17

manager   ~0051399

Good point @dregad. We should also based on this discussion update the documentation of the events in the developer manual to clarify our recommendation for plugin authors.

dregad

dregad

2015-09-08 04:29

developer   ~0051401

@syncguru,

Mantis is largely a content app. The JS is very lightly used

You're right of course, but we also have to consider where we'll be in a year or two.

Mantis is an application that was designed 15 years ago. Until very recently, it was even a system requirement to be able to fully operate without relying on any javascript at all.

We are slowly moving away from that; starting with 1.3, javascript will no longer be optional. With 2.0 and the new UI, I'm sure we will rely more and more on dynamic contents.

So even though we will most likely never become a "heavy JS app", we should ask ourselves whether (and by how much) our reliance on javascript will increase in the coming years.

@vboctor

Of course, if we persist in loading javascript at page bottom, we need to update doc (and also make this change clear in release notes to the attention of plugin authors so they can update their code)

atrol

atrol

2015-09-09 02:37

developer   ~0051410

One more issue introduced by 0013285, see 0020088.
Maybe it's better to revert 0013285 to avoid more delay to get out 1.3.0 RC1
We could try/discuss this again in 2.0.x

dregad

dregad

2015-09-09 02:45

developer   ~0051411

With this additional issue, I think the best course of action at this time is to revert.

bkraul

bkraul

2015-09-10 10:25

reporter   ~0051437

Just a thought from a humble developer, and I apologize if this has been mentioned before. It is good that jQuery is being included in the installation. However, jQuery constantly updates. Wouldn't it make more sense to keep it as a plugin, just like the Formatting or Graphs plugins? or even make it a core one? This would make it easier for developers to update the version if desired, without having to look in the mantis system directories.

vboctor

vboctor

2015-09-10 22:25

manager   ~0051444

+1 for reverting.

@bkraul, upgrading jquery whether it is in a plugin or not, is pretty much the same. It is currently a dependency for core and is treated in a consistent way with the rest of the libraries that we depend on.

dregad

dregad

2015-09-11 11:31

developer   ~0051453

I reverted the change.

For the record, this also impacted the source integration plugin's search page.

Related Changesets

MantisBT: master 8d7ea37e

2015-09-11 06:03

dregad


Details Diff
Revert move-js-to-footer

This change (see issue 0013282) introduced several regressions.

This reverts commit 2dbc86b10d10495f1312b781fe0a3519a0d7152a, reversing
changes made to aa9b4f8ad7908325633542840805c4c854adb1f1.

Fixes 0020081, 0020088
Affected Issues
0013285, 0020081, 0020088
mod - core/html_api.php Diff File