Skip to content

introduce $$rest #4489

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 10 commits into from
Mar 10, 2020
Merged

introduce $$rest #4489

merged 10 commits into from
Mar 10, 2020

Conversation

tanhauhau
Copy link
Member

Fixes #2930

Based on @Conduitry's comment #3015 (comment)

@tanhauhau tanhauhau force-pushed the tanhauhau/rest-props branch from 3ab4396 to 232988f Compare March 1, 2020 02:58
const rest = {};
keys = new Set(keys);
for (const k in props) if (!keys.has(k) && k[0] !== '$') rest[k] = props[k];
return Object.freeze(rest);
Copy link
Member

Choose a reason for hiding this comment

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

Is there a particular reason to freeze this when $$props isn't frozen?

Copy link
Member Author

Choose a reason for hiding this comment

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

I just think that $$rest shouldn't be mutated, because it's kind of derived from $$props, and mutation on $$rest will not propagate back to the $$props

Copy link
Member

Choose a reason for hiding this comment

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

It doesn't have to be ironed out before this can get merged, but what are your thoughts on allowing $$props to be mutated? I think that right now it can but those changes won't be reflected in the corresponding other variables.

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually, initially I was thinking of only freezing in dev mode, to educate users not to mutate it.

Not sure Object.freeze has any performance penalties...

What do u think?

Copy link
Member Author

Choose a reason for hiding this comment

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

On another thought, I remove the freeze for now, to be consistent with the $$props.

In a separate PR, I'll add warnings for assignment or mutation on $$props and $$rest

@Conduitry
Copy link
Member

We currently disable the dev-mode 'component was created with unknown prop foo' warning when a component uses $$props and I think we should probably do the same when it uses $$rest.

@Conduitry
Copy link
Member

I see in #2930 that $$attrs was suggested because it sounds like the name of something in Vue. I don't think that having the same name as Vue for a somewhat similar feature is necessarily a good idea. I'm not married to $$rest either though.

@tanhauhau
Copy link
Member Author

I'm not familiar with vue, but reading their docs on $attr, https://vuejs.org/v2/guide/components-props.html#Disabling-Attribute-Inheritance, it is not exactly the same concept of what we have..

Naming is hard, that's why I just use whatever you suggested...

Maybe some other suggestions:

  • $$unknownProps
  • $$restProps

Since it's a local variable, I reckon it's minifyable, long names should be fine.

@tanhauhau tanhauhau force-pushed the tanhauhau/rest-props branch from 0ef3ecc to 7d4ecba Compare March 5, 2020 01:01
@PatrickG PatrickG mentioned this pull request Mar 9, 2020
@Conduitry Conduitry force-pushed the tanhauhau/rest-props branch from 3548cef to 77c393e Compare March 10, 2020 00:44
@Conduitry Conduitry merged commit 91d758e into sveltejs:master Mar 10, 2020
@Conduitry
Copy link
Member

Merged, thank you! I've removed the tutorial page for now because if we're going to mention $$restProps in the tutorial we should probably also mention $$props, and if we're mentioning both, maybe they should be on the same page, or at least one should build on the other.

sthagen added a commit to sthagen/sveltejs-svelte that referenced this pull request Mar 10, 2020
@tanhauhau tanhauhau deleted the tanhauhau/rest-props branch March 10, 2020 15:03
@jimmywarting
Copy link
Contributor

Wow, this is weird stuff... just came to know about this throught some google search.
perks of learning a new framework

taylorzane pushed a commit to taylorzane/svelte that referenced this pull request Dec 17, 2020
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.

Feature request: group unknown props for forwarding
3 participants