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

docs(props): Immutability & Signals explanation and example. #6460

Merged
merged 8 commits into from
Jun 8, 2024

Conversation

codyroberts
Copy link
Contributor

@codyroberts codyroberts commented Jun 5, 2024

Overview

The props section now clarifies that props are immutable and cannot be changed, and recommends readers to use signals in situations where the props need to be updated.

The example now shows two separate ways to use signals for props.

What is it?

  • Feature / enhancement
  • Bug
  • Docs / tests / types / typos

Description

Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context. List any dependencies that are required for this change.

Use cases and why

    1. Explains prop immutability
    1. Recommends the use of signals
    1. Gives examples of signal use

Checklist:

  • My code follows the developer guidelines of this project
  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation
  • Added new tests to cover the fix / functionality

The props section now clarifies that props are immutable and cannot be changed, and recommends readers to use signals in situations where the props need to be updated.

The example now shows two separate ways to use signals for props.
@codyroberts codyroberts requested review from a team as code owners June 5, 2024 03:57
Copy link

netlify bot commented Jun 5, 2024

👷 Deploy request for qwik-insights pending review.

Visit the deploys page to approve it

Name Link
🔨 Latest commit 77db77f

@codyroberts codyroberts changed the title Added Immutability and Signals to the explanation and example. docs: Added Immutability and Signals to the explanation and example. Jun 5, 2024
@codyroberts codyroberts changed the title docs: Added Immutability and Signals to the explanation and example. docs: Added Immutability and Signals to the props explanation and example. Jun 5, 2024
Copy link
Member

@wmertens wmertens 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 your contribution, improving the docs is always welcome, but props should generally not be signals.

An exception is the bind:value prop.

@@ -113,40 +113,46 @@ export default component$(() => {

## Props

Props are used to pass data from the parent into the component. Props are accessible via the `props` argument to the component$ function.
Props are used to pass data from the parent into the component. They are accessible via the `props` argument to the `component$` function. Data passed as props is immutable, and cannot be changed. In cases where data sent as props needs to be updated, it is recommended to use signals.
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 not actually the case. The data is mutable, but unless you use signals or stores, there won't be a reactive change.
The props object itself is private to the component, it can also be mutated without problems, which might be handy when using props spreading.

Copy link
Contributor

Choose a reason for hiding this comment

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

@codyroberts maybe changing "immutable" to "non reactive" would be more accurate in that case?

@wmertens can you give a short example of the props spreading benefit please?

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 tested it locally and I am still getting Error: Code(17): props are immutable when doing something simple like:

export const Item = component$<ItemProps>((props) => {
  props.price = 10;
  return (
    <ul>
      <li>name: {props.name}</li>
      <li>quantity: {props.quantity}</li>
      <li>description: {props.description}</li>
      <li>price: {props.price}</li>
    </ul>
  );
});

I was making this change to the documentation in response to this thread:
#6423

Copy link
Member

Choose a reason for hiding this comment

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

@codyroberts ah indeed you're right, I didn't notice that before. So indeed the props object is shallowly immutable, but not deeply. This does not error:

import { component$ } from '@builder.io/qwik';

export const Foo = component$((props: any) => {
  // mutate a given prop
  props.bar.hello = 3

  return <button {...props}>A button</button>
})

export default component$(() => {
  return <Foo bar={{ hi: true }} />;
});

So deleting props isn't possible, you need to override via the props order.


In this example a component `Item` declares optional `name`, `quantity`, `description`, and `price`.
In the example below a component `Item` declares optional `name`, `quantity`, `description`, and `price` props. The `quantity` and `price` props are signals; `price` is a signal before being sent to `Item` and can be changed from the parent component. `quantity` is sent as a number value, and a new signal is created from the initial value inside of the `Item` component, where it can be changed.
Copy link
Member

Choose a reason for hiding this comment

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

No, this is actually not a good idea. You should deference signals when you put them into props, and the optimizer will forward them.

Using signals as props is possible but it complicates the mental model and actual code without benefits.

Copy link
Contributor

Choose a reason for hiding this comment

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

@codyroberts so in Qwik, there's an optimization happening behind the scenes which allows you to use someProp={mySignal.value}

This won't actually create a subscription, but will pass the signal down for you and let you use that value wherever you need to use it and the subscription will be created for you.

@wmertens btw, it'll be "read only", meaning you can't really change that signal when you pass it's value as a prop.

If you want the ability to change it you need to pass it unwrapped (as the example shows) and then you can modify it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh I see now, thank you!

If necessary I could probably make the documentation clearer about the fact that if you don't want to use a signal and don't need to modify the original value then you can create a local variable and set it to the props value, then change that.

Copy link
Member

Choose a reason for hiding this comment

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

"I could probably make the documentation clearer" yes please

Copy link
Contributor

Choose a reason for hiding this comment

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

If necessary I could probably make the documentation clearer about the fact that if you don't want to use a signal and don't need to modify the original value then you can create a local variable and set it to the props value, then change that.

Thanks!
yeah most of the time if you just need the prop value you'd probably just de-structure it like:

component$(({something}: MyProps) => {
...
} )

// OR

const { something } = props;

That's what mostly we're seeing

Demo for the reference props example in the props section of the component documentation.
Added a demo for the primitive props example in the props section of the component documentation.
Made the properties on the details object in the example optional to match the primitive props example.
Rewrote the props section to clarify shallow immutability, and to give examples of both primitive and reference data types being sent as props with explanations of the difference in immutability.
@codyroberts
Copy link
Contributor Author

@shairez @wmertens @PatrickJS

I tried to clarify the things we talked about, and added a couple examples and corresponding demos. Please let me know what you think!

Considered making the examples use different items, but decided to keep them the same so that changes in code functionality are more clear.

Changed data back to match examples in the documentation.
@PatrickJS
Copy link
Member

I'm good with everything so long as everyone else is

Copy link
Member

@gioboa gioboa left a comment

Choose a reason for hiding this comment

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

Thanks for your help 👏

@PatrickJS PatrickJS changed the title docs: Added Immutability and Signals to the props explanation and example. docs(props): Immutability & Signals explanation and example. Jun 7, 2024
@PatrickJS PatrickJS enabled auto-merge (squash) June 7, 2024 20:29
@shairez
Copy link
Contributor

shairez commented Jun 8, 2024

LGTM
Thanks @codyroberts ! 🙏

Copy link

pkg-pr-new bot commented Jun 8, 2024

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

@builder.io/qwik (77db77f)

npm i https://pkg.pr.new/@builder.io/qwik@6460

@builder.io/qwik-city (77db77f)

npm i https://pkg.pr.new/@builder.io/qwik-city@6460

eslint-plugin-qwik (77db77f)

npm i https://pkg.pr.new/eslint-plugin-qwik@6460

create-qwik (77db77f)

npm i https://pkg.pr.new/create-qwik@6460

@shairez shairez closed this Jun 8, 2024
auto-merge was automatically disabled June 8, 2024 00:26

Pull request was closed

@shairez shairez reopened this Jun 8, 2024
@shairez shairez merged commit 133ff1b into QwikDev:main Jun 8, 2024
38 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

5 participants