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

Reprocess elements after specific changes are made to them #156

Open
wants to merge 11 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
16 changes: 16 additions & 0 deletions CONTRIBUTING.md
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,22 @@ To only run the JavaScript tests, open your console and run `yarn test` from the

To only run the Ruby tests, open your console and run `rake test:helpers` from the project directory.

### Integration tests

Integration tests are run through `yarn` using [Playwright](https://github.com/microsoft/playwright) for browser testing. Browser and runtime configuration can be found in [`playwright.config.js`](./playwright.config.js).

To begin testing, install the browser drivers:

```bash
yarn playwright install --with-deps
```

Then, run the tests:

```bash
yarn test:integration
```

## Testing specific time zones

Stubbing the browser's time zone is fragile. Although we have some automated tests for specific time zones, we also need to do some manual testing as follows:
Expand Down
2 changes: 1 addition & 1 deletion app/assets/javascripts/local-time.es2017-esm.js

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion app/assets/javascripts/local-time.es2017-umd.js

Large diffs are not rendered by default.

23 changes: 19 additions & 4 deletions lib/assets/javascripts/src/local-time/controller.coffee
Original file line number Diff line number Diff line change
@@ -1,14 +1,21 @@
import LocalTime from "./local_time"
import "./relative_time"
import "./page_observer"
import "./element_observations"

{parseDate, strftime, getI18nValue, config} = LocalTime

class LocalTime.Controller
SELECTOR = "time[data-local]:not([data-localized])"
SELECTOR = "time[data-local]"
NON_LOCALIZED_SELECTOR = "#{SELECTOR}:not([data-localized])"

constructor: ->
@pageObserver = new LocalTime.PageObserver SELECTOR, @processElements
@observations = new LocalTime.ElementObservations(SELECTOR, @processElement)
@pageObserver = new LocalTime.PageObserver(
elementAddedSelector: NON_LOCALIZED_SELECTOR,
elementAddedCallback: @processElements,
elementRemovedSelector: SELECTOR,
elementRemovedCallback: @disconnectObserver)

start: ->
unless @started
Expand All @@ -21,11 +28,11 @@ class LocalTime.Controller
if interval = config.timerInterval
@timer ?= setInterval(@processElements, interval)

processElements: (elements = document.querySelectorAll(SELECTOR)) =>
processElements: (elements = document.querySelectorAll(NON_LOCALIZED_SELECTOR)) =>
@processElement(element) for element in elements
elements.length

processElement: (element) ->
processElement: (element) =>
datetime = element.getAttribute("datetime")
local = element.getAttribute("data-local")
format = if config.useFormat24
Expand Down Expand Up @@ -57,6 +64,14 @@ class LocalTime.Controller
when "weekday-or-date"
relative(time).toWeekdayString() or relative(time).toDateString()

@connectObserver(element)

connectObserver: (element) =>
@observations.include(element)

disconnectObserver: (element) =>
@observations.disregard(element)

markAsLocalized = (element) ->
element.setAttribute("data-localized", "")

Expand Down
52 changes: 52 additions & 0 deletions lib/assets/javascripts/src/local-time/element_observations.coffee
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
import LocalTime from "./local_time"

{elementMatchesSelector} = LocalTime

class LocalTime.ElementObservations
OBSERVABLE_ATTRIBUTES = [ "datetime", "data-local", "data-format", "data-format24" ]

constructor: (@selector, @callback) ->
@elements = new Map()

include: (element) =>
unless @elements.get(element)
observer = @observe(element)
@register(element, observer)

disregard: (element) =>
if observer = @elements.get(element)?.observer
observer.disconnect()
@elements.delete(element)

observe: (element) =>
observer = new MutationObserver(@processMutations)
observer.observe(element, characterData: true, subtree: true, attributes: true, attributeFilter: OBSERVABLE_ATTRIBUTES)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Per Mozilla, setting textContent (which is what this library uses to update the element) removes all of the node's children and replaces them with a single text node with the given string value. So it shouldn't cause infinite loops due to this configuration.

This was also my experience while testing. But of course we'll want to thoroughly test on our apps before a public release.

observer

register: (element, observer) =>
@elements.set(element, { observer: observer, updates: -1 })
@incrementUpdates(element)

processMutations: (mutations) =>
for mutation in mutations
target = mutation.target

element = if target.nodeType is Node.TEXT_NODE
target.parentNode
else
target
Comment on lines +34 to +37
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Turbo morphs could change either/or — the data attributes on the element itself, or the value of the nested text node


if elementMatchesSelector(element, @selector)
@processLingeringElement(element)
break

processLingeringElement: (element) =>
@incrementUpdates(element)
@callback(element)

incrementUpdates: (element) =>
@elements.get(element).updates++
element.setAttribute("data-updates", @elements.get(element).updates)
Comment on lines +47 to +49
Copy link
Contributor Author

@josefarias josefarias Dec 6, 2023

Choose a reason for hiding this comment

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

Only done for testing purposes.

We mutate the element as a response to mutations, so this PR is prone to infinite loops if we're not careful. I think asserting the number of updates is a good protection against this.


size: ->
@elements.size
47 changes: 28 additions & 19 deletions lib/assets/javascripts/src/local-time/page_observer.coffee
Original file line number Diff line number Diff line change
Expand Up @@ -3,43 +3,52 @@ import LocalTime from "./local_time"
{elementMatchesSelector} = LocalTime

class LocalTime.PageObserver
constructor: (@selector, @callback) ->
constructor: ({
elementAddedSelector,
elementAddedCallback,
elementRemovedSelector,
elementRemovedCallback
}) ->
@elementAddedSelector = elementAddedSelector
@elementAddedCallback = elementAddedCallback
@elementRemovedSelector = elementRemovedSelector
@elementRemovedCallback = elementRemovedCallback

start: ->
unless @started
@observeWithMutationObserver() or @observeWithMutationEvent()
@observe()
@started = true

observeWithMutationObserver: ->
if MutationObserver?
observer = new MutationObserver @processMutations
observer.observe(document.documentElement, childList: true, subtree: true)
true

observeWithMutationEvent: ->
addEventListener("DOMNodeInserted", @processInsertion, false)
true

findSignificantElements: (element) ->
elements = []
if element?.nodeType is Node.ELEMENT_NODE
elements.push(element) if elementMatchesSelector(element, @selector)
elements.push(element.querySelectorAll(@selector)...)
elements
observe: ->
observer = new MutationObserver @processMutations
observer.observe(document.documentElement, childList: true, subtree: true)

processMutations: (mutations) =>
elements = []

for mutation in mutations
switch mutation.type
when "childList"
for node in mutation.addedNodes
elements.push(@findSignificantElements(node)...)

for node in mutation.removedNodes
if elementMatchesSelector(node, @elementRemovedSelector)
@elementRemovedCallback(node)

@notify(elements)

findSignificantElements: (element) ->
elements = []
if element?.nodeType is Node.ELEMENT_NODE
elements.push(element) if elementMatchesSelector(element, @elementAddedSelector)
elements.push(element.querySelectorAll(@elementAddedSelector)...)
elements

processInsertion: (event) =>
elements = @findSignificantElements(event.target)
@notify(elements)

notify: (elements) ->
if elements?.length
@callback?(elements)
@elementAddedCallback?(elements)
6 changes: 5 additions & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -15,9 +15,12 @@
},
"homepage": "https://github.com/basecamp/local_time",
"devDependencies": {
"@playwright/test": "^1.40.1",
"@rollup/plugin-alias": "^5.0.0",
"@rollup/plugin-node-resolve": "^15.1.0",
"@rollup/plugin-terser": "^0.4.3",
"@web/test-runner-playwright": "^0.11.0",
"chai": "^4.3.10",
"coffeescript": "^2.7.0",
"express": "^4.18.2",
"rollup": "^3.25.1",
Expand All @@ -27,6 +30,7 @@
"start": "node test/javascripts/server.mjs",
"build": "rollup -c",
"watch": "rollup -wc",
"test": "yarn build && yarn start"
"test": "yarn build && yarn start",
"test:integration": "yarn build && playwright test"
}
}
44 changes: 44 additions & 0 deletions playwright.config.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
import { devices } from "@playwright/test"

const config = {
projects: [
{
name: "chrome",
use: {
...devices["Desktop Chrome"],
contextOptions: {
timeout: 5000
}
}
},
{
name: "firefox",
use: {
...devices["Desktop Firefox"],
contextOptions: {
timeout: 5000
}
}
},
{
name: "webkit",
use: {
...devices["Desktop Safari"],
contextOptions: {
timeout: 5000
}
}
}
],
browserStartTimeout: 60000,
testDir: "./test/javascripts/",
testMatch: /integration\/.*_test\.js/,
webServer: {
command: "yarn start"
},
use: {
baseURL: "http://localhost:9000/"
}
}

export default config
67 changes: 67 additions & 0 deletions test/javascripts/fixtures/integration_tests.html
Original file line number Diff line number Diff line change
@@ -0,0 +1,67 @@
<!DOCTYPE html>
<head>
<meta charset="utf-8">
<title>Integration Tests</title>
<script src="/current-build"></script>
</head>
<body>
<script>
LocalTime.start()

function simulateMorph() {
const textNode = document.getElementById("one").firstChild
textNode.nodeValue = "changed"
}
Comment on lines +11 to +14
Copy link
Contributor Author

@josefarias josefarias Dec 6, 2023

Choose a reason for hiding this comment

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


function changeDatetime() {
const element = document.getElementById("one")
element.setAttribute("datetime", "2015-12-04T14:00:00Z")
}

function changeToRelative() {
const element = document.getElementById("one")
element.setAttribute("data-local", "time-ago")
}

function changeFormat() {
const element = document.getElementById("one")
element.setAttribute("data-format", "%b %e")
}

function changeFormat24() {
LocalTime.config.useFormat24 = true
LocalTime.run()

const element = document.getElementById("one")
element.setAttribute("data-format24", "%B %e")
}

function changeFoo() {
const element = document.getElementById("one")
element.setAttribute("data-foo", "bar")
}

function reprocess() {
LocalTime.run()
}

function removeElement() {
const element = document.getElementById("one")
element.parentNode.removeChild(element)
}
</script>

<button id="morph" onclick="simulateMorph()">Morph</button>
<button id="change-datetime" onclick="changeDatetime()">Change Datetime</button>
<button id="change-relative" onclick="changeToRelative()">Change To Relative</button>
<button id="change-format" onclick="changeFormat()">Change Format</button>
<button id="change-format24" onclick="changeFormat24()">Change Format24</button>
<button id="change-foo" onclick="changeFoo()">Change Foo</button>

<button id="reprocess" onclick="reprocess()" style="background-color: darkseagreen;">Reprocess</button>
<button id="remove" onclick="removeElement()" style="background-color: firebrick; color: white;">Remove Element</button>

<br>

<time id="one" datetime="2024-12-04T14:00:00Z" data-format="%B %e, %Y" data-local="time"></time>
</body>
Loading