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

Add doc for URL binding feature #1346

Merged
merged 1 commit into from
Jan 17, 2024

Conversation

squrious
Copy link
Contributor

Q A
Bug fix? no
New feature? no
Issues NA
License MIT

Add documentation for #1230

@squrious squrious force-pushed the live-component/docs-url-binding branch 2 times, most recently from c600ad6 to 0adbb66 Compare December 20, 2023 10:39
Copy link
Member

@kbond kbond left a comment

Choose a reason for hiding this comment

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

Looks good! Here's a couple questions I had after reading the doc:

  • Is the browser's history updated?
  • What happens if multiple components on the same page use this feature? Should this be avoided?

@squrious
Copy link
Contributor Author

Is the browser's history updated?

No, the current behavior is to update the URL in place. There could be a feature to push new history entries in the future.

What happens if multiple components on the same page use this feature? Should this be avoided?

If multiple components use this feature on the same page, things should work fine as long as they don't use the same field names. Otherwise parameters will collide, so the last rendered component will take priority on URL parameters, overriding ones set by the other components.

Should this be part of the documentation too?

@kbond
Copy link
Member

kbond commented Dec 21, 2023

Should this be part of the documentation too?

Maybe just a warning


.. note::

Type consistency is enforced by the LiveComponent internal hydrator when the component is initialized with query
Copy link
Collaborator

Choose a reason for hiding this comment

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

Type consistency is enforced by the LiveComponent internal hydrator. When the component is initialized, If a query parameter could not be hydrated properly into a LiveProp, it will be ignored to avoid unfriendly errors for the end user.

Just a suggestion.. feel free to ignore if you prefer the original ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I prefer the original, I find it more accurate as this type consistency check is only used on component initialization. But thanks for the suggestion :)

Copy link
Member

Choose a reason for hiding this comment

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

I think we should make this not in a note, rather a normal sentence, as it's really important. And, from a user's point of view, the important thing is that it goes through the hydration system. Something like:

When a page is loaded with a query parameter that's bound to a LiveProp (e.g. /search?query=my+search+string), the value - my search string - goes through the hydration system before it's set onto the property. If a value can't be hydrated, it will be ignored.

src/LiveComponent/doc/index.rst Outdated Show resolved Hide resolved
Validation
~~~~~~~~~~

To validate your component state when it is rendered for the first time, you have to setup a `PostMount hook`_::
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd use a warning or caution block there.. wdyt ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added a caution block at the top of the section

Copy link
Member

Choose a reason for hiding this comment

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

setup -> set up

Copy link
Member

@weaverryan weaverryan left a comment

Choose a reason for hiding this comment

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

Thank you for this! What a great way to start 2024, being reminded that we can to release this feature soon.


.. versionadded:: 2.13

The ``url`` option was introduced in Live Components 2.13.
Copy link
Member

Choose a reason for hiding this comment

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

2.14

@@ -2302,6 +2302,131 @@ You can also trigger a specific "action" instead of a normal re-render:
#}
>

Binding LiveProp to URL query parameters
Copy link
Member

Choose a reason for hiding this comment

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

How about:

Changing the URL when a LiveProp changes


The ``url`` option was introduced in Live Components 2.13.

You can bind your component LiveProp's values to URL query parameters thanks to the ``url`` option::
Copy link
Member

Choose a reason for hiding this comment

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

How about:

If you want the URL to update when a LiveProp changes, you can do that with the url option:


You can bind your component LiveProp's values to URL query parameters thanks to the ``url`` option::

// src/Components/SearchModule.php
Copy link
Member

Choose a reason for hiding this comment

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

    // src/Twig/Components/SearchModule.php

right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Other examples in the doc live in src/Components, not sure why this one should be different?

public string $query = '';
}

Now if you change the value of the ``query`` prop, the url will be updated to reflect the new state of your
Copy link
Member

Choose a reason for hiding this comment

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

Now, when the user changes the...

... prop, a query parameter in the URL will be updated...

parameters. If a value could not be hydrated properly, it will be ignored to avoid unfriendly errors for the end
user.

Multiple bindings
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Multiple bindings
Multiple Query Parameter Bindings


And you only set the ``query`` value, then your URL will be updated to ``https://my.domain/search?query=my+query+string&mode=fulltext``.

Validation
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Validation
Validating the Query Parameter Values

Validation
~~~~~~~~~~

To validate your component state when it is rendered for the first time, you have to setup a `PostMount hook`_::
Copy link
Member

Choose a reason for hiding this comment

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

setup -> set up

public function postMount(): void
{
// Validate 'mode' field without throwing an exception
$this->validateField('mode', false);
Copy link
Member

Choose a reason for hiding this comment

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

It's not clear to me what happens when this fails.

.. caution::

It is recommended to validate data provided through query string parameters, so you can prevent malicious inputs
from being processed and rendered.
Copy link
Member

Choose a reason for hiding this comment

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

This is interesting. If a LiveProp is writable, then the fact that it is bound to a query parameter does not open a new malicious entrypoint: the value can always be changed to "anything". Two things:

A) If we want to talk about validating writable properties in general more clearly, then we can talk about that in another PR
B) This section may still make sense, but I think it's only important for the (probably rare) situation where you have url: true and writable: false (e.g. when the user calls a LiveAction, you change the prop inside that method, but the user doesn't change it directly).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The goal of this section was to be explicit about the fact that bound props are not validated when initialized from the URL, even if the component uses validation. But yes, the security concern only applies when the validation logic itself involves some security process (sanitizing or whatever). Maybe I can change the caution block to a note like:

If your component uses validation, it is recommended to also validate data provided through query string parameters.

For the B) part, I could add another example in this section to illustrate this use case, wdyt?

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, ok, yes, it's more clear to me. So the messaging would be something like:

Like any writable LiveProp, because the user can modify this value, you should consider adding validation (<-- link to that section). When you bind a LiveProp to the URL, the initial value is not automatically validated. To validate it: (then the code).

Also, it's beyond the scope of the doc, but should the URL-binding feature validate automatically? And reject the value (skip it) if it's invalid?

@weaverryan weaverryan force-pushed the live-component/docs-url-binding branch from 7a740f6 to 4f0d313 Compare January 17, 2024 15:39
@weaverryan
Copy link
Member

Thank you @squrious!

@weaverryan weaverryan merged commit d0c201d into symfony:2.x Jan 17, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants