Replace filters with an extendibility API to hook into Cart and Checkout blocks #3774
Conversation
Aljullu
added
the
skip-changelog
|
Size Change: +338 B (0%) Total Size: 1.15 MB
ℹ️ View Unchanged
|
Aljullu
changed the title
Aljullu
added
category: extensibility
| const newValue = filter( value, args ); | ||
| value = validate( newValue ) ? newValue : value; | ||
| } ); | ||
| return value; |
There was a problem hiding this comment.
This works in this case because we have one filter: totalLabel.
But if there were more filters the value returned would be of the last filter applied. Is this the expected result, or should the filters have some sort of priority?
There was a problem hiding this comment.
Currently, that's an accepted tradeoff, we don't plan on introducing priority for the time.
| 'totalLabel', | ||
| __( 'Total', 'woo-gutenberg-products-block' ), | ||
| { | ||
| extensions, | ||
| }, | ||
| __experimentalValidateElementOrString |
There was a problem hiding this comment.
Using an object would make reading this much easier.
packages/checkout/index.js
Outdated
| export * from './registry'; | ||
| export * from './registry/validations.js'; |
There was a problem hiding this comment.
'./registry/validations.js' would probably make more sense to be inside utils
There was a problem hiding this comment.
Given the changes in 094eea7 (we only accept strings for this filter), I ended up removing that file altogether.
packages/checkout/registry/index.js
Outdated
| const filters = getCheckoutFilters( filterName ); | ||
| let value = defaultValue; | ||
| filters.forEach( ( filter ) => { | ||
| const newValue = filter( value, args ); |
There was a problem hiding this comment.
This is too dangerous to just execute like this, any malfunction in the 3PD code would break our block.
You can test this by changing extensions to extension in WCS code, the whole cart block is now broken.
The error can be at runtime or can happen with a version mismatch (so the 3PD import something that is no longer available) or just anything.
A very important principle when developing the new extensibility points is to not trust the developer.
The filter call should be wrapped in a try/catch closure that will skip that particular filter something wrong happens.
Skipping filters is only possible on raw values, we can't know for sure if an element is faulty or not until we evaluate it, meaning we would break other extensions.
We could wrap our __experimentalApplyCheckoutFilter call in a BlockErrorBoudary but that would still neglect the rest of filters.
We can also nest all filters into each other somehow, but I'm not sure how would that happen.
I'd suggest we skip element for now and only have raw values until we have a strong case for elements and then we can think about it very well.
packages/checkout/registry/index.js
Outdated
| let value = defaultValue; | ||
| filters.forEach( ( filter ) => { | ||
| const newValue = filter( value, args ); | ||
| value = validate( newValue ) ? newValue : value; |
There was a problem hiding this comment.
validate only checks the returned value but doesn't guarantee no error was thrown before reaching the return point.
There was a problem hiding this comment.
Should be fixed in the commits linked in the previous comment.
senadir
force-pushed
the
fix/3735-filters-api
branch
from
7493826 to
e2eb926
Compare
|
@Aljullu I rebased this PR to trunk so it wouldn't conflict more (and to resolve conflict on my PR). |
Aljullu
force-pushed
the
fix/3735-filters-api
branch
from
cf84ca4 to
73cb8e4
Compare
|
Thanks for the feedback @ralucaStan and @senadir. I made several changes to apply the feedback, so I think this is ready for another look. |
| // eslint-disable-next-line no-console | ||
| console.log( e ); |
There was a problem hiding this comment.
Logging errors is not very practical, it wouldn't trigger an error on the console (easily missed) and would happen to all users or admins.
I will write a P2 to suggest how we can catch and expose errors, for now, how about you check if the current use is an admin, fi so, you throw, or else you neglect the error and move on.
| // eslint-disable-next-line no-console | |
| console.log( e ); | |
| if ( CURRENT_USER_IS_ADMIN ) { | |
| throw e; | |
| } |
There was a problem hiding this comment.
We will probably need to revisit this after the P2 discussion, but for now I made the change you suggested (throw when the user is an admin) but still kept the console.error for costumers: 1712766. Let me know how it looks.
| test( 'should return default value if there are no filters', () => { | ||
| const value = 'Hello World'; | ||
| const newValue = __experimentalApplyCheckoutFilter( { | ||
| filterName, | ||
| defaultValue: value, | ||
| } ); | ||
|
|
||
| expect( newValue ).toBe( value ); | ||
| } ); |
There was a problem hiding this comment.
Another test we can add (we need to mock CURRENT_USER_IS_ADMIN) is passing in invalid code (that throws an exception) and making sure previous filter still passed.
There was a problem hiding this comment.
I had a bad time mocking CURRENT_USER_IS_ADMIN so it had different values in two different tests. I ended up splitting them in two files: 1d5b636. There might better ways of doing this (let me know if you find how!), but couldn't figure it out how after spending some time on this.
Fixes #3735.
This PR refactors the filter added in #3716 so instead of using
@wordpress/hooks, it uses our own API. This adds an abstraction layer that should make life easier for extensions and it allows us to more easily make internal changes without breaking 3rd party extensions.There are a couple of PRs that will need to be updated if we go ahead with this approach: #3750 and #3763. cc @opr
How to test the changes in this Pull Request:
fix/filters-api(see associated PR: 3979-gh-woocommerce/woocommerce-subscriptions).Totallabel isTotal due today.Total due todaylabel changes toTotal.