Mark unsafe forwarded props#2496
Conversation
|
ugh sheesh that's annoying. And doing it this way will create a duplication of knowledge right? the props in the inner component and the unsafe listing in the outer component. Is there no better way...?? @claude ? (do we even have claude on this repo lol?) |
|
I summoned Claude locally, it should chime in. The only potentially-slightly-better way would be to have more knowledge of flux:delegate-component and passing through attributes (recursively) in Blaze. Not a big fan of more coupling with flux and tracking of recursive attribute forwarding is a mess and a performance impact, I had that implemented at some point. |
|
Hey @calebporzio — I looked at this PR and the Blaze source code in depth. Here's my understanding and thoughts: The root causeIn Blaze's
This assumption breaks when a component delegates to Is there a better way?The duplication concern is valid — every component that uses A potential Blaze-level feature that could help: something like @blaze(fold: true, unsafe_from: ['flux::with-field'])This would tell Blaze: "look up The outer components would go from this: @blaze(fold: true, unsafe: [
'name', 'label', 'badge',
'description', 'description:trailing',
'label:badge', 'label:aside', 'label:trailing',
'error:name', 'error:bag', 'error:message', 'error:icon', 'error:nested', 'error:deep',
])To this: @blaze(fold: true, unsafe_from: ['flux::with-field'])Other alternatives I considered:
Bottom lineThis PR is correct and solves the bug. If the duplication bothers you, |
|
Yeah something |
|
well...clearly we can't punt on this because it's currently a bug...but...I don't like any of the solutions haha. Adding Marking nested attribute forwarding props I think is better even though it's bad lol. And I don't think too many people forward attributes in the way that we do, but i could be wrong. From a DX perspective, intelligently accounting for this without making the dev think about it is the only way. Another valid option is somehow getting away from with-field attribute forwarding and such. Maybe get away from with-field in general? idk... I have no solutions, but I can at least prevent or slow us from making a bad choice. |
|
I don't like the solutions either lol. The one applied in this PR is the least offensive imo, it just makes maintainability worse. But it's an actual fix that we can merge, doesn't require adding any special APIs. We just need to keep this in mind when adding new properties to with-fields or variants. Perhaps we could come up with an automated way to check this? I don't think the attribute forwarding we do here is common at all. In user-land you would just disable fold for a component that does this.
This is the ideal fix, but it would require a massive rewrite, probably best to leave for Flux v3. My recommendation would be:
Agreed, if this was in user-land. But this is an obscure need of Flux (and could be eliminated). Tracking of the props is a huge lift, adds extra file reads, computation and makes compilation slower. For making a better DX of something only we deal with. |
|
Ok, yeah is there something automated or almost automated we could do? there are other things like this. like checking that the right free/pro components are in the publish command list (which I'm now realizing i might not have added timeline and slider lol). And that like kinda thing is in the flux docs for the pro components. you know anything that could drift, it would be nice to have a setup for. maybe we have a few /check-*** skills that get run either for PRs or pre-release? probably makes the most sense for them to be either post-merge or pre-release so that they don't take too long or too many tokens and hold up PRs. We probably need some kind of automated release tool at this point. (we're manually checking the composer.json flux/flux-pro version matches as well) So maybe we make a single /prep-release claude skill that runs through all these things. whadyathink? |
|
I like the idea of automated checks a lot. I'll put something together. But I would try to make them deterministic, the release process should be 100% reliable I believe. Agree? |
|
meh, idk. but deterministic is definitely better if possible, so not gonna fight you on that |
The scenario
When using Blaze, attributes forwarded through
<flux:with-field>or<flux:delegate-component>don't work correctly when passed as dynamic PHP variables:The above always renders
ui-labeldespite$labelbeingnull.The problem
Blaze treats props as "safe" (foldable at compile time) when they're not declared in the component's own
@props. Forwarded props likelabel,description,error:name, etc. are defined in a different file (with-field.blade.php), so Blaze can't see them and folds them as static values.The solution
Manually declare all forwarded props as
unsafein the@blazedirective of each affected component.Fixes #2458