Skip to content

Commit

Permalink
feature #1216 [Live] Throwing an error when setting an invalid model …
Browse files Browse the repository at this point in the history
…name (weaverryan)

This PR was merged into the 2.x branch.

Discussion
----------

[Live] Throwing an error when setting an invalid model name

| Q             | A
| ------------- | ---
| Bug fix?      | no
| New feature?  | yes
| Tickets       | From a Slack conversation
| License       | MIT

Currently, if you use an invalid prop/model name in `data-model`, there is no error: the field is just silently *not* set. Now, we throw a clear error.

It's possible (?) that people were relying on this - notably if someone is using a `<form data-model="*">` to auto-map every form field as a model... but only some exist on their component. However, 99% of the time people are using that with a Symfony form + `ComponentWithFormTrait`, where that will not be an issue. And I think if you have `data-model="*"`, you are saying that *every* field is a prop. For the sake of DX, we should fail loudly and not let users make accidents.

Cheers!

Commits
-------

ed68c9c [Live] Throwing an error when setting an invalid model name
  • Loading branch information
weaverryan committed Nov 7, 2023
2 parents 1db34a8 + ed68c9c commit 7b90f56
Show file tree
Hide file tree
Showing 3 changed files with 16 additions and 1 deletion.
3 changes: 3 additions & 0 deletions src/LiveComponent/assets/dist/live_controller.js
Original file line number Diff line number Diff line change
Expand Up @@ -1799,6 +1799,9 @@ class Component {
set(model, value, reRender = false, debounce = false) {
const promise = this.nextRequestPromise;
const modelName = normalizeModelName(model);
if (!this.valueStore.has(modelName)) {
throw new Error(`Invalid model name "${model}".`);
}
const isChanged = this.valueStore.set(modelName, value);
this.hooks.triggerHook('model:set', model, value, this);
this.unsyncedInputsTracker.markModelAsSynced(modelName);
Expand Down
4 changes: 4 additions & 0 deletions src/LiveComponent/assets/src/Component/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -160,6 +160,10 @@ export default class Component {
set(model: string, value: any, reRender = false, debounce: number|boolean = false): Promise<BackendResponse> {
const promise = this.nextRequestPromise;
const modelName = normalizeModelName(model);

if (!this.valueStore.has(modelName)) {
throw new Error(`Invalid model name "${model}".`);
}
const isChanged = this.valueStore.set(modelName, value);

this.hooks.triggerHook('model:set', model, value, this);
Expand Down
10 changes: 9 additions & 1 deletion src/LiveComponent/assets/test/Component/index.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ const makeTestComponent = (): { component: Component, backend: MockBackend } =>
const component = new Component(
document.createElement('div'),
'test-component',
{ firstName: '' },
{ firstName: '', product: { name: '' } },
[],
() => [],
null,
Expand Down Expand Up @@ -65,6 +65,14 @@ describe('Component class', () => {
// @ts-ignore
expect(await backendResponse?.getBody()).toEqual('<div data-live-props-value="{}"></div>');
});

it('errors when an invalid model is passed', async () => {
const { component } = makeTestComponent();

// setting nested - totally ok
component.set('product.name', 'Ryan', false);
expect(() => { component.set('notARealModel', 'Ryan', false) }).toThrow('Invalid model name "notARealModel"');
});
});

describe('Proxy wrapper', () => {
Expand Down

0 comments on commit 7b90f56

Please sign in to comment.