Skip to content
This repository has been archived by the owner on Jan 10, 2024. It is now read-only.

Breaks when Turbolinks is enabled. #109

Closed
george-carlin opened this issue Mar 17, 2014 · 8 comments
Closed

Breaks when Turbolinks is enabled. #109

george-carlin opened this issue Mar 17, 2014 · 8 comments

Comments

@george-carlin
Copy link

Visiting the page /specs works fine the first time - but when I click any link (e.g. to run one specific spec instead of them) the page breaks with the Javascript error Uncaught Error: jquery-ujs has already been loaded!.

The problem is Turbolinks. Turbolinks reloads everything between the <body> tags without causing a full page refresh, so any <script> tags within the body are going to be reloaded every time a link is clicked - causing bugs and errors like the one described here.

As Turbolinks is now included in Rails by default, this is potentially going to be a time-wasting issue for a lot of people.

Solutions I see:

  1. Undo this commit: e0a8446
  2. Alter the method jasmine_js_files (here) to prevent turbolinks.js from being included in the output script tags:
    def jasmine_js_files
      files = Jasmine::Core.js_files
      files << jasmine_boot_file
      if params[:console]
        files << 'jasmine-console-shims.js'
        files << 'jasmine-console-reporter.js'
      end
      files << 'jasmine-specs.js'
      files.delete "turbolinks.js" # Is it this simple, or am I missing something?
      files
    end
  1. Some other way I haven't thought of. Is it possible to disable turbolinks in /specs only? I don't like the idea of removing turbolinks from the test gem group because someone might have a project where, for whatever reason, their integration tests are dependent on turbolinks being present.
@searls
Copy link
Member

searls commented Apr 12, 2014

Looking into this now

@searls
Copy link
Member

searls commented Apr 12, 2014

I'm not able to reproduce this locally, but I'm glad to revert e0a8446, since there's no added value to sticking JS at the bottom of the body if the document has no content.

@searls searls closed this as completed in 26ec940 Apr 12, 2014
@jcoglan
Copy link

jcoglan commented Dec 4, 2014

@searls This commit broke our test suite; we have code that does this:

$('body').on('click', element, function() { ... });

to take advantage of jQuery's event delegation. Problem is, with this change, the code now runs before <body> exists, so a bunch of tests fail for no good reason.

Placement of JS is not an incidental detail, and I think most people would expect it to be loaded after the body is ready. When I'm writing my own JS test pages (which I would normally do but I didn't configure this project), I always place scripts at the end of the document, as they typically would be in production.

If this interacts badly with Turbolinks, I would argue this is a problem with Turbolinks (it causes numerous problems that it's unreasonable to expect all frontend tool authors to work around).

@searls
Copy link
Member

searls commented Dec 4, 2014

Ugh, thanks @jcoglan for the clear description of the issue this raises for you.

In all my years of jQuery'ing, however, I've always made a point to stick after event handlers (even those delegated off body inside a domready callback.

$(function(){
  $('body').on('click', element, function() { ... });
});

Is not doing so intentional on your team's part? I like the idea of using jQuery selections before domready only slightly more than I like the idea of using turbolinks.

@george-carlin
Copy link
Author

"I think most people would expect it to be loaded after the body is ready"

^^^ This may be true in the general case, but it's absolutely not true in Rails, as the default Rails behaviour is to wrap up all your JS into <%= javascript_include_tag "application" %> which gets put in your <head>. Doubly true now that Rails includes turbolinks by default so putting <script> tags inside your body is a recipe for disaster unless you disable turbolinks.

Not sure what the ideal solution is here but I still think that including jasmine js files in the head makes much sense than in the body.

@jasonkarns
Copy link
Member

Since this gem is rails-specific, I would suggest relying on the default Rails behavior.

However, I believe there is a solution to accommodate both: the defer attribute.

Script would still go in <head> to please turbolinks. And with the defer attribute, the script won't be executed until the DOM is ready (domready wrapper not needed).

@searls
Copy link
Member

searls commented Dec 4, 2014

A couple thoughts at this point, @jcoglan (beyond our editorialization that generally binding in jQuery before domready isn't good):

  1. You could fix this in your app with a custom template that makes only this change. See the README for an example on this (scroll to the part where they override the template)
  2. jasmine-rails can probably safely add the deferred property to each javascript tag and resolve this issue indirectly. I'm playing with that now.

searls added a commit to searls/jasmine-rails-exploratory-tests that referenced this issue Dec 4, 2014
@searls
Reproduces @jcoglan's issue from testdouble/jasmine-rails#109
4045ff4
searls added a commit that referenced this issue Dec 4, 2014
@searls
Add "defer" to all javascript tags
e41486f 
This should resolve the issue @jcoglan experienced in #109
@searls searls mentioned this issue Dec 4, 2014
Merged
@searls
Copy link
Member

searls commented Dec 4, 2014

The defer fix landed just now in 0.10.3. Hopefully this resolves @jcoglan's issue

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@jcoglan @searls @jasonkarns @george-carlin