Skip to content

Make autoloading changes visible to the DOM #57

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.

Sign up for GitHub

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

Closed
seanpdoyle wants to merge 2 commits into from
Closed

Make autoloading changes visible to the DOM #57

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
422acd0
Make autoloading changes visible to the DOM
seanpdoyle Apr 9, 2021
6a3e518
Update mimemagic
seanpdoyle Apr 9, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions Gemfile.lock
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,9 @@ GEM
marcel (0.3.3)
mimemagic (~> 0.3.2)
method_source (1.0.0)
mimemagic (0.3.5)
mimemagic (0.3.10)
nokogiri (~> 1)
rake
mini_mime (1.0.2)
mini_portile2 (2.5.0)
minitest (5.14.4)
Expand Down Expand Up @@ -180,4 +182,4 @@ DEPENDENCIES
webdrivers

BUNDLED WITH
2.2.5
2.2.12
32 changes: 29 additions & 3 deletions app/assets/javascripts/stimulus/loaders/autoloader.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,30 @@ const application = Application.start()
const { controllerAttribute } = application.schema
const registeredControllers = {}

function createTokenList(element, attribute) {
const tokenList = document.createElement("div").classList
const tokens = element.getAttribute(attribute) || ""
tokens.split(/\s+/).filter(content => content.length).forEach(token => tokenList.add(token))

return tokenList
}

function addToTokenList(element, attribute, value) {
const tokenList = createTokenList(element, attribute)
tokenList.add(value)
element.setAttribute(attribute, tokenList.toString())
}

function removeFromTokenList(element, attribute, value) {
const tokenList = createTokenList(element, attribute)
tokenList.remove(value)
if (tokenList.length) {
element.setAttribute(attribute, tokenList.toString())
} else {
element.removeAttribute(attribute)
}
}

function autoloadControllersWithin(element) {
queryControllerNamesWithin(element).forEach(loadController)
}
Expand All @@ -13,13 +37,16 @@ function queryControllerNamesWithin(element) {
}

function extractControllerNamesFrom(element) {
return element.getAttribute(controllerAttribute).split(/\s+/).filter(content => content.length)
const tokenList = createTokenList(element, controllerAttribute)
return Array.from(tokenList).map(name => ({ element, name }))
Copy link
Copy Markdown
Contributor Author

@seanpdoyle seanpdoyle Apr 9, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe storing the currently autoloader identifiers on the html, body, or a <meta> element would suffice, instead of on each individual element.

Copy link
Copy Markdown

@mgodwin mgodwin Apr 9, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What are your thoughts around placing it on html, body or meta? I was thinking that with a data-stimulus-autoloading attribute on the element directly you could at least target that with a css class and have a path to display a loading indicator that way (or block interaction).

}

function loadController(name) {
function loadController({ element, name }) {
addToTokenList(element, "data-stimulus-autoloading", name)
import(controllerFilename(name))
.then(module => registerController(name, module))
.catch(error => console.log(`Failed to autoload controller: ${name}`, error))
.finally(() => removeFromTokenList(element, "data-stimulus-autoloading", name))
}

function controllerFilename(name) {
Expand All @@ -33,7 +60,6 @@ function registerController(name, module) {
registeredControllers[name] = true
}


new MutationObserver((mutationsList) => {
for (const { attributeName, target, type } of mutationsList) {
switch (type) {
Expand Down
6 changes: 4 additions & 2 deletions test/dummy/app/assets/javascripts/controllers/hello_controller.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,9 @@
import { Controller } from "stimulus"

export default class extends Controller {
connect() {
this.element.textContent = "Hello World!"
static get targets() { return [ "input", "output" ] }

greet() {
this.outputTarget.innerHTML = `Hello, ${this.inputTarget.value}`
}
}
4 changes: 4 additions & 0 deletions test/dummy/app/assets/javascripts/controllers/namespace/message_rendering_controller.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,4 +6,8 @@ export default class extends Controller {
connect() {
this.element.innerHTML = `Namespace: ${this.messageValue}`
}

sayHello() {
this.element.innerHTML = `Hello from Namespace: ${this.messageValue}`
}
}
9 changes: 9 additions & 0 deletions test/dummy/app/views/application/index.html.erb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,15 @@
<%= tag.p "", data: { namespace__message_rendering_message_value: params[:message], controller: "
namespace--message-rendering
" } %>

<div data-controller="hello">
<input type="text" value="Waiting for Eager Load" data-hello-target="input">
<div data-hello-target="output"></div>

<button type="button" data-action="click->hello#greet">
Say Hello
</button>
</div>
</section>

<section id="load-attribute" data-controller="loading" data-loading-controller-value="message-rendering" data-message-rendering-message-value="<%= params[:message] %>">
Expand Down
20 changes: 20 additions & 0 deletions test/system/autoload_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,16 @@ class AutoloadTest < ApplicationSystemTestCase
end
end

test "waits for autoloading to complete" do
visit root_path

within "#eager-loaded" do
click_button "Say Hello"

assert_text "Hello, Waiting for Eager Load"
end
end

test "autoloads Controller modules on the page lazily" do
visit root_path(message: "Hello World!")

Expand Down Expand Up @@ -45,4 +55,14 @@ class AutoloadTest < ApplicationSystemTestCase
assert_text "Namespace: Hello, from Turbo page"
end
end

def click_button(*)
Copy link
Copy Markdown
Contributor Author

@seanpdoyle seanpdoyle Apr 9, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In its current state, this draft PR only extends the click_button method, but if we find this approach viable, we'd want to extend all methods in Capybara::DSL.

Copy link
Copy Markdown

@mgodwin mgodwin Apr 9, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a super interesting way to address this problem! One thing that I'm wrestling with is that I can't help but feel like this behavior codifies the possible experience for a user where:

  1. they click on a button, it doesn't work (controller hasn't loaded yet)
  2. they wait for a second, click on it again, and it does work (after the controller has loaded)

Granted, in most cases, the delay here should be negligible and shouldn't be a problem, but I do think there are situations where this will come up outside of an automated test scenario.

Just an idea here, but I wonder if it'd be possible to specify strategies for how to deal with this in the library that would help avoid the need to override this behavior in capybara, e.g. data-controller-autoload="hide" or data-controller-autoload="spinner" - in the former case, you could just wait for the element to become visible to click on it (which capybara already does pretty well), and in the latter, you could handle that with a helper method somewhere. By making it an explicit attribute, I think it could help illustrate to devs that this node may render before the javascript behavior has been attached to it, and you may need to deal with that.

begin
assert_css "[data-stimulus-autoloading]"
Copy link
Copy Markdown
Contributor Author

@seanpdoyle seanpdoyle Apr 9, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This assertion is intended to wait for the autoloading to start.

Copy link
Copy Markdown
Contributor Author

@seanpdoyle seanpdoyle Apr 9, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be best to wrap this in a using_wait_time block of 0-ish to cut down on time spent waiting.

rescue
# does not eagerly load any Stimulus
end
assert_no_css "[data-stimulus-autoloading]"
Copy link
Copy Markdown
Contributor Author

@seanpdoyle seanpdoyle Apr 9, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This assertion is intended to wait for the autoloading to complete.

If we think using assert calls would make the test suite assertion count fluctuate too much, there might be other ways to wait on the presence or absence of elements matching the selector.

super
end
end