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

Convert Filter resolution from property to accessor #7769

Merged
merged 5 commits into from
Sep 13, 2021

Conversation

mcdenhoed
Copy link
Contributor

@mcdenhoed mcdenhoed commented Sep 2, 2021

Description of change

This PR is an attempt to fix this issue that I've had with the PIXI Advanced Blur Filter (documented here: pixijs/filters#301). I describe the issue in detail there, but here's a short summary:

Filter has a public property named resolution. AdvancedBloomFilter overrides this property with an accessor named resolution. AdvancedBloomFilter maintains a private _resolution property which is set by this accessor (along with some other side-effects, which seems to be the whole purpose of using an accessor instead of a property).

However, as of microsoft/TypeScript#37894, tsc now treats overriding a property with an accessor as an error.

This PR converts the definition of resolution in the base Filter class to an accessor to avoid this error.

Pre-Merge Checklist
  • Lint process passed (npm run lint)
  • Tests passed (npm run test)

packages/core/src/filters/Filter.ts Outdated Show resolved Hide resolved
@bigtimebuddy bigtimebuddy added this to the v6.1.3 milestone Sep 8, 2021
@ShukantPal ShukantPal added the ✅ Ready To Merge Helpful when issues are in the queue waiting to get merged. This means the PR is completed and has t label Sep 10, 2021
@bigtimebuddy bigtimebuddy merged commit 0e45893 into pixijs:dev Sep 13, 2021
@bigtimebuddy
Copy link
Member

Thank you @mcdenhoed!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
✅ Ready To Merge Helpful when issues are in the queue waiting to get merged. This means the PR is completed and has t
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants