Skip to content

Conversation

@trueadm
Copy link
Contributor

@trueadm trueadm commented Oct 17, 2024

This should allow the pattern I mention in this comment:

#13649

I also added a layer of protection and validation in DEV to ensure people don't try and pass in arbitrary functions pretending to be snippets.

I think this would be impactful for migration but might have other benefits too. @dummdidumm does this check out to you?

@changeset-bot
Copy link

changeset-bot bot commented Oct 17, 2024

⚠️ No Changeset found

Latest commit: 5e7bd0d

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

protect

protect
@paoloricciuti
Copy link
Member

I also added a layer of protection and validation in DEV to ensure people don't try and pass in arbitrary functions pretending to be snippets.

I mean this is already possible right?

@dummdidumm
Copy link
Member

I fear this will result in false positives, since it's possible to have a prop and slot with the same name. I'd rather make it a convention that dashes are converted to underscores during the migration

@paoloricciuti
Copy link
Member

I fear this will result in false positives, since it's possible to have a prop and slot with the same name. I'd rather make it a convention that dashes are converted to underscores during the migration

But then we can't really migrate those slots because we might not migrate the receiving component. I think it's fine too, we can just error and output the migrate task

var possible_snippet = $$props[name];
if (
typeof possible_snippet === 'function' &&
(!DEV || validated_snippets.has(possible_snippet))
Copy link
Member

Choose a reason for hiding this comment

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

That feels brittle to me - it means things work subtly different in prod, which could break things.

@dummdidumm
Copy link
Member

But then we can't really migrate those slots because we might not migrate the receiving component.

We can. What I mean is, instead of just always checking for a prop, we convert dashes to underscores before checking $$slots once more.

This PR needs a test btw.

@paoloricciuti
Copy link
Member

We can. What I mean is, instead of just always checking for a prop, we convert dashes to underscores before checking $$slots once more.

Ohhh gotcha...i'll work on the rest of the stuff for the migration while we make a decision on this.

@trueadm
Copy link
Contributor Author

trueadm commented Oct 17, 2024

Yeah I think it's fine to go a different direction, I'll close this PR.

@trueadm trueadm closed this Oct 17, 2024
@dummdidumm dummdidumm deleted the slot-props branch October 18, 2024 06:08
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.

4 participants