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

chore: removal of default prop values #163

Merged
merged 8 commits into from
Jan 12, 2025

Conversation

Tinoooo
Copy link
Contributor

@Tinoooo Tinoooo commented Jan 6, 2025

To enable the prop watcher helper to retrieve default constructor values from plain effects, we must not set default values in the effect component's props ourselves. This MR removes the default values from the prop definitions in the affected effects.

@Tinoooo Tinoooo self-assigned this Jan 6, 2025
@Tinoooo Tinoooo requested a review from alvarosabu as a code owner January 6, 2025 21:07
Copy link

pkg-pr-new bot commented Jan 10, 2025

Open in Stackblitz

npm i https://pkg.pr.new/@tresjs/post-processing@163

commit: 1273cde

Copy link
Member

@alvarosabu alvarosabu left a comment

Choose a reason for hiding this comment

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

Thanks @Tinoooo left a couple of comments before merging

offset: () => new Vector2(0.01, 0.01),
radialModulation: false,
modulationOffset: 0.15,
radialModulation: undefined,
Copy link
Member

Choose a reason for hiding this comment

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

why do we set it to undefined here? Wouldn't be better not to use withDefaults here @Tinoooo ?

Copy link
Contributor Author

@Tinoooo Tinoooo Jan 11, 2025

Choose a reason for hiding this comment

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

We have to set it to undefined to prevent Vue from casting it to boolean when the prop is not in use. Without this line, we have no chance to find out whether the prop was actively set or not.

@@ -16,8 +17,7 @@ export interface NoisePmndrsProps {

<script lang="ts" setup>
const props = withDefaults(defineProps<NoisePmndrsProps>(), {
premultiply: false,
blendFunction: BlendFunction.SCREEN,
premultiply: undefined,
Copy link
Member

Choose a reason for hiding this comment

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

same here, wouldn't be better to remove the withDefault all along?

Copy link
Contributor Author

@Tinoooo Tinoooo Jan 11, 2025

Choose a reason for hiding this comment

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

We have to set it to undefined to prevent Vue from casting it to boolean when the prop is not in use. Without this line, we have no chance to find out whether the prop was actively set or not.

@Tinoooo Tinoooo requested a review from alvarosabu January 12, 2025 12:58
@alvarosabu alvarosabu merged commit 78b0bb7 into main Jan 12, 2025
4 checks passed
@Tinoooo Tinoooo deleted the chore/removal-of-default-prop-values branch January 12, 2025 16:46
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.

2 participants