Skip to content

Fix transforms on assets with applicability#668

Merged
sugarmanz merged 4 commits intomainfrom
fix-applicability-transforms
Jun 24, 2025
Merged

Fix transforms on assets with applicability#668
sugarmanz merged 4 commits intomainfrom
fix-applicability-transforms

Conversation

@sugarmanz
Copy link
Copy Markdown
Member

@sugarmanz sugarmanz commented Jun 24, 2025

Will close #667

Change Type (required)

Indicate the type of change your pull request is:

  • patch
  • minor
  • major
  • N/A

Does your PR have any documentation updates?

  • Updated docs
  • No Update needed
  • Unable to update docs

Release Notes

Move AssetTransformCorePlugin application after applicability and switch view plugins to ensure transforms properly apply to conditional assets.

📦 Published PR as canary version: 0.11.3--canary.668.23668

Try this version out locally by upgrading relevant packages to 0.11.3--canary.668.23668

@sugarmanz sugarmanz requested a review from brocollie08 as a code owner June 24, 2025 20:13
@sugarmanz sugarmanz added the patch Increment the patch version when merged label Jun 24, 2025
@sugarmanz sugarmanz changed the title Fix applicability transforms Fix transforms on assets with applicability Jun 24, 2025
Comment thread core/player/src/view/plugins/index.ts Outdated
export { default as SwitchPlugin } from "./switch";
export { default as MultiNodePlugin } from "./multi-node";
export { default as AssetPlugin } from "./asset";
export * from "./asset-transform";
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Do we need a star export here if there is only one exported target from this file?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Also, maybe a question for another time, but all the other internal plugins are default exports, should this follow suite for consistency or should we change them all now?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I thought we were doing away w/ default exports — so, I left as-is even though it didn't follow the other plugins. I don't care one way or another, I'll just change it for consistency and we can revert the commit otherwise.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yeah I'd say we probably should update these as well. I can make an issue for that.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

KetanReddy
KetanReddy previously approved these changes Jun 24, 2025
@sugarmanz
Copy link
Copy Markdown
Member Author

/canary

@sugarmanz
Copy link
Copy Markdown
Member Author

/canary

@codecov
Copy link
Copy Markdown

codecov Bot commented Jun 24, 2025

Codecov Report

Attention: Patch coverage is 72.38095% with 29 lines in your changes missing coverage. Please review.

Project coverage is 90.24%. Comparing base (7a36733) to head (f44744e).
Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
core/player/src/view/plugins/asset-transform.ts 71.56% 29 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #668      +/-   ##
==========================================
- Coverage   90.25%   90.24%   -0.01%     
==========================================
  Files         333      333              
  Lines       20606    20605       -1     
  Branches     1992     1991       -1     
==========================================
- Hits        18597    18595       -2     
- Misses       1993     1994       +1     
  Partials       16       16              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@sugarmanz sugarmanz enabled auto-merge (squash) June 24, 2025 22:20
@sugarmanz sugarmanz merged commit 570c426 into main Jun 24, 2025
10 of 11 checks passed
@sugarmanz sugarmanz deleted the fix-applicability-transforms branch June 24, 2025 22:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

patch Increment the patch version when merged

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Applicability on assets prevents beforeResolve transforms from executing

2 participants