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

Reconsider usage of accessor properties #349

Closed
vanruesc opened this issue Feb 23, 2022 · 3 comments
Closed

Reconsider usage of accessor properties #349

vanruesc opened this issue Feb 23, 2022 · 3 comments
Labels
enhancement Enhancement of existing functionality

Comments

@vanruesc
Copy link
Member

Is your feature request related to a problem?

Recent API refactoring replaced accessor properties and plain properties with methods. This will inadvertently increase the maintenance burden for maintainers of react-posprocessing if left as-is.

Describe the solution you'd like

Reconsider using a hybrid approach of methods and accessors properties if applicable.

Additional context

#279 (comment)

@vanruesc vanruesc added the enhancement Enhancement of existing functionality label Feb 23, 2022
@vanruesc vanruesc mentioned this issue Feb 23, 2022
22 tasks
@drcmda
Copy link
Member

drcmda commented Feb 23, 2022

I'll copy the middle in here for reference:

we won't be able to set and, more importantly, spread props over passes, we'll have to know all props beforehand and translate them to functions. functions cannot be expressed declaratively, they are not serializable either.

good examples could be threejs, or the dom, they strike a sensible balance between atomics and more complex functions that will either carry out calculations or set multiple props at once. the upside for atomics is that they can easily by driven by state models and spread over the surface.

another upside is maintenance for abstractions of pp. if pp updates and introduces a new atomic, say foo, the userland solution or library using it just needs to npm update and foo will work without having to adjust the abstraction.

we are also publishing types for raw postprocessing, https://github.com/pmndrs/react-postprocessing/blob/master/types/postprocessing.d.ts that would be another "abstraction" that's currently quite simple to adjust.

@vanruesc
Copy link
Member Author

good examples could be threejs, or the dom, they strike a sensible balance between atomics and more complex functions that will either carry out calculations or set multiple props at once.

That sounds reasonable. One motivation behind the move towards methods was that a lot of settings on effects need to trigger shader recompilations to take effect. Using method calls makes it clear that changes will be expensive as opposed to just changing the value of a seemingly harmless property.

vanruesc added a commit that referenced this issue Mar 7, 2022
vanruesc added a commit that referenced this issue Mar 9, 2022
@vanruesc
Copy link
Member Author

vanruesc commented Mar 9, 2022

Getter and setter methods have been converted into accessor properties in [email protected]. I had my concerns regarding accessors, but it's ultimately ease-of-use and consistency that matters.

@vanruesc vanruesc closed this as completed Mar 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Enhancement of existing functionality
Projects
None yet
Development

No branches or pull requests

2 participants