Skip to content

[Live] Always send HTML over the wire instead of JSON #245

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

Merged
merged 1 commit into from
Jan 25, 2022
Merged
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
5 changes: 5 additions & 0 deletions src/LiveComponent/CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,5 +1,10 @@
# CHANGELOG

## 2.1.0

- The Live Component AJAX endpoints now return HTML in all situations
instead of JSON.

## 2.0.0

- Support for `stimulus` version 2 was removed and support for `@hotwired/stimulus`
Expand Down
49 changes: 12 additions & 37 deletions src/LiveComponent/assets/src/live_controller.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,12 +6,6 @@ import { buildFormData, buildSearchParams } from './http_data_helper';
import { setDeepData, doesDeepPropertyExist, normalizeModelName } from './set_deep_data';
import { haveRenderedValuesChanged } from './have_rendered_values_changed';

interface LiveResponseData {
redirect_url?: string,
html?: string,
data?: any,
}

interface ElementLoadingDirectives {
element: HTMLElement,
directives: Directive[]
Expand Down Expand Up @@ -195,20 +189,8 @@ export default class extends Controller {
this._makeRequest(null);
}

_getValueFromElement(element: HTMLElement){
const value = element.dataset.value || element.value;

if (!value) {
const clonedElement = (element.cloneNode());
// helps typescript know this is an HTMLElement
if (!(clonedElement instanceof HTMLElement)) {
throw new Error('cloneNode() produced incorrect type');
}

throw new Error(`The update() method could not be called for "${clonedElement.outerHTML}": the element must either have a "data-value" or "value" attribute set.`);
}
Copy link
Member Author

Choose a reason for hiding this comment

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

Unrelated bug fix from a feature I merged recently #203. That PR added this check, but it IS ok to have an empty value - it just means, for example, that a textarea is empty! For some reason, the tests didn't catch this before, but after this change, one test DID start failing.


return value;
_getValueFromElement(element: HTMLElement) {
return element.dataset.value || element.value;
}

_updateModelFromElement(element: HTMLElement, value: string, shouldRender: boolean) {
Expand Down Expand Up @@ -320,7 +302,7 @@ export default class extends Controller {

const fetchOptions: RequestInit = {};
fetchOptions.headers = {
'Accept': 'application/vnd.live-component+json',
'Accept': 'application/vnd.live-component+html',
};

if (action) {
Expand Down Expand Up @@ -351,8 +333,8 @@ export default class extends Controller {

const isMostRecent = this.renderPromiseStack.removePromise(thisPromise);
if (isMostRecent) {
response.json().then((data) => {
this._processRerender(data)
response.text().then((html) => {
this._processRerender(html, response);
});
}
})
Expand All @@ -363,24 +345,24 @@ export default class extends Controller {
*
* @private
*/
_processRerender(data: LiveResponseData) {
_processRerender(html: string, response: Response) {
// check if the page is navigating away
if (this.isWindowUnloaded) {
return;
}

if (data.redirect_url) {
if (response.headers.get('Location')) {
// action returned a redirect
if (typeof Turbo !== 'undefined') {
Turbo.visit(data.redirect_url);
Turbo.visit(response.headers.get('Location'));
} else {
window.location.href = data.redirect_url;
window.location.href = response.headers.get('Location');
}

return;
}

if (!this._dispatchEvent('live:render', data, true, true)) {
if (!this._dispatchEvent('live:render', html, true, true)) {
// preventDefault() was called
return;
}
Expand All @@ -390,15 +372,8 @@ export default class extends Controller {
// elements to appear different unnecessarily
this._onLoadingFinish();

if (!data.html) {
throw new Error('Missing html key on response JSON');
}

// merge/patch in the new HTML
this._executeMorphdom(data.html);

// "data" holds the new, updated data
this.dataValue = data.data;
this._executeMorphdom(html);
}

_clearWaitingDebouncedRenders() {
Expand Down Expand Up @@ -644,7 +619,7 @@ export default class extends Controller {
this.pollingIntervals.push(timer);
}

_dispatchEvent(name: string, payload: object | null = null, canBubble = true, cancelable = false) {
_dispatchEvent(name: string, payload: object | string | null = null, canBubble = true, cancelable = false) {
return this.element.dispatchEvent(new CustomEvent(name, {
bubbles: canBubble,
cancelable,
Expand Down
8 changes: 4 additions & 4 deletions src/LiveComponent/assets/test/controller/action.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -48,10 +48,10 @@ describe('LiveController Action Tests', () => {
const { element } = await startStimulus(template(data));

// ONLY a post is sent, not a re-render GET
const postMock = fetchMock.postOnce('http://localhost/_components/my_component/save', {
html: template({ comments: 'hi weaver', isSaved: true }),
data: { comments: 'hi weaver', isSaved: true }
});
const postMock = fetchMock.postOnce(
'http://localhost/_components/my_component/save',
template({ comments: 'hi weaver', isSaved: true })
);

await userEvent.type(getByLabelText(element, 'Comments:'), ' WEAVER');

Expand Down
1 change: 1 addition & 0 deletions src/LiveComponent/assets/test/controller/child.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -133,6 +133,7 @@ describe('LiveController parent -> child component tests', () => {
const inputElement = getByLabelText(element, 'Content:');
await userEvent.clear(inputElement);
await userEvent.type(inputElement, 'changed content');
// change the rows on the server
mockRerender({'value': 'changed content'}, childTemplate, (data) => {
data.rows = 5;
});
Expand Down
8 changes: 4 additions & 4 deletions src/LiveComponent/assets/test/controller/csrf.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -47,10 +47,10 @@ describe('LiveController CSRF Tests', () => {
const data = { comments: 'hi' };
const { element } = await startStimulus(template(data));

const postMock = fetchMock.postOnce('http://localhost/_components/my_component/save', {
html: template({ comments: 'hi', isSaved: true }),
data: { comments: 'hi', isSaved: true }
});
const postMock = fetchMock.postOnce(
'http://localhost/_components/my_component/save',
template({ comments: 'hi', isSaved: true })
);
getByText(element, 'Save').click();

await waitFor(() => expect(element).toHaveTextContent('Comment Saved!'));
Expand Down
39 changes: 25 additions & 14 deletions src/LiveComponent/assets/test/controller/model.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -41,10 +41,7 @@ describe('LiveController data-model Tests', () => {
const data = { name: 'Ryan' };
const { element, controller } = await startStimulus(template(data));

fetchMock.getOnce('end:?name=Ryan+WEAVER', {
html: template({ name: 'Ryan Weaver' }),
data: { name: 'Ryan Weaver' }
});
fetchMock.getOnce('end:?name=Ryan+WEAVER', template({ name: 'Ryan Weaver' }));

await userEvent.type(getByLabelText(element, 'Name:'), ' WEAVER', {
// this tests the debounce: characters have a 10ms delay
Expand All @@ -66,10 +63,7 @@ describe('LiveController data-model Tests', () => {
const data = { name: 'Ryan' };
const { element, controller } = await startStimulus(template(data));

fetchMock.getOnce('end:?name=Jan', {
html: template({ name: 'Jan' }),
data: { name: 'Jan' }
});
fetchMock.getOnce('end:?name=Jan', template({ name: 'Jan' }));

userEvent.click(getByText(element, 'Change name to Jan'));

Expand All @@ -96,12 +90,11 @@ describe('LiveController data-model Tests', () => {
['guy', 150]
];
requests.forEach(([string, delay]) => {
fetchMock.getOnce(`end:my_component?name=Ryan${string}`, {
// the _ at the end helps us look that the input has changed
// as a result of a re-render (not just from typing in the input)
html: template({ name: `Ryan${string}_` }),
data: { name: `Ryan${string}_` }
}, { delay });
fetchMock.getOnce(
`end:my_component?name=Ryan${string}`,
template({ name: `Ryan${string}_` }),
{ delay }
);
});

await userEvent.type(getByLabelText(element, 'Name:'), 'guy', {
Expand Down Expand Up @@ -275,4 +268,22 @@ describe('LiveController data-model Tests', () => {
// assert all calls were done the correct number of times
fetchMock.done();
});

it('data changed on server should be noticed by controller', async () => {
const data = { name: 'Ryan' };
const { element, controller } = await startStimulus(template(data));

mockRerender({name: 'Ryan WEAVER'}, template, (data) => {
// sneaky server changes the data!
data.name = 'Kevin Bond';
});

const inputElement = getByLabelText(element, 'Name:');
await userEvent.type(inputElement, ' WEAVER');

await waitFor(() => expect(inputElement).toHaveValue('Kevin Bond'));
expect(controller.dataValue).toEqual({name: 'Kevin Bond'});

fetchMock.done();
});
});
27 changes: 11 additions & 16 deletions src/LiveComponent/assets/test/controller/render.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -61,12 +61,11 @@ describe('LiveController rendering Tests', () => {
const data = { name: 'Ryan' };
const { element } = await startStimulus(template(data));

fetchMock.get('http://localhost/_components/my_component?name=Ryan', {
html: template({ name: 'Kevin' }),
data: { name: 'Kevin' }
}, {
delay: 100
});
fetchMock.get(
'http://localhost/_components/my_component?name=Ryan',
template({ name: 'Kevin' }),
{ delay: 100 }
);
getByText(element, 'Reload').click();
userEvent.type(getByLabelText(element, 'Comments:'), '!!');

Expand All @@ -84,12 +83,11 @@ describe('LiveController rendering Tests', () => {
template(data, true)
);

fetchMock.get('http://localhost/_components/my_component?name=Ryan', {
html: template({ name: 'Kevin' }, true),
data: { name: 'Kevin' }
}, {
delay: 100
});
fetchMock.get(
'http://localhost/_components/my_component?name=Ryan',
template({ name: 'Kevin' }, true),
{ delay: 100 }
);
getByText(element, 'Reload').click();
userEvent.type(getByLabelText(element, 'Comments:'), '!!');

Expand All @@ -102,10 +100,7 @@ describe('LiveController rendering Tests', () => {
const data = { name: 'Ryan' };
const { element } = await startStimulus(template(data));

fetchMock.get('end:?name=Ryan', {
html: '<div>aloha!</div>',
data: { name: 'Kevin' }
}, {
fetchMock.get('end:?name=Ryan', '<div>aloha!</div>', {
delay: 100
});

Expand Down
5 changes: 1 addition & 4 deletions src/LiveComponent/assets/test/tools.ts
Original file line number Diff line number Diff line change
Expand Up @@ -74,10 +74,7 @@ const mockRerender = (sentData, renderCallback, changeDataCallback = null) => {
changeDataCallback(sentData);
}

fetchMock.mock(url, {
html: renderCallback(sentData),
data: sentData
});
fetchMock.mock(url, renderCallback(sentData));
}

export { startStimulus, getControllerElement, initLiveComponent, mockRerender };
29 changes: 5 additions & 24 deletions src/LiveComponent/src/EventListener/LiveComponentSubscriber.php
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@

use Psr\Container\ContainerInterface;
use Symfony\Component\EventDispatcher\EventSubscriberInterface;
use Symfony\Component\HttpFoundation\JsonResponse;
use Symfony\Component\HttpFoundation\Request;
use Symfony\Component\HttpFoundation\Response;
use Symfony\Component\HttpKernel\Event\ControllerEvent;
Expand Down Expand Up @@ -41,8 +40,7 @@
*/
class LiveComponentSubscriber implements EventSubscriberInterface, ServiceSubscriberInterface
{
private const JSON_FORMAT = 'live-component-json';
private const JSON_CONTENT_TYPE = 'application/vnd.live-component+json';
private const HTML_CONTENT_TYPE = 'application/vnd.live-component+html';

private ContainerInterface $container;

Expand All @@ -69,8 +67,6 @@ public function onKernelRequest(RequestEvent $event): void
return;
}

$request->setFormat(self::JSON_FORMAT, self::JSON_CONTENT_TYPE);

// the default "action" is get, which does nothing
$action = $request->get('action', 'get');
$componentName = (string) $request->get('component');
Expand Down Expand Up @@ -192,16 +188,17 @@ public function onKernelResponse(ResponseEvent $event): void
return;
}

if (!$this->isLiveComponentJsonRequest($request)) {
if (!\in_array(self::HTML_CONTENT_TYPE, $request->getAcceptableContentTypes(), true)) {
return;
}

if (!$response->isRedirection()) {
return;
}

$event->setResponse(new JsonResponse([
'redirect_url' => $response->headers->get('Location'),
$event->setResponse(new Response(null, 204, [
'Location' => $response->headers->get('Location'),
'Content-Type' => self::HTML_CONTENT_TYPE,
]));
}

Expand All @@ -227,27 +224,11 @@ private function createResponse(object $component, Request $request): Response
$request->attributes->get('_component_template')
);

if ($this->isLiveComponentJsonRequest($request)) {
return new JsonResponse(
[
'html' => $html,
'data' => $this->container->get(LiveComponentHydrator::class)->dehydrate($component),
],
200,
['Content-Type' => self::JSON_CONTENT_TYPE]
);
}

return new Response($html);
}

private function isLiveComponentRequest(Request $request): bool
{
return 'live_component' === $request->attributes->get('_route');
}

private function isLiveComponentJsonRequest(Request $request): bool
{
return \in_array($request->getPreferredFormat(), [self::JSON_FORMAT, 'json'], true);
}
}
Loading