Enhancement/8687 pax date range #8701
Conversation
|
Build files for 296e040 have been deleted. |
| const date = formatPaxDate( '2019-10-31' ); | ||
|
|
||
| expect( date.year ).toBe( 2019 ); | ||
| expect( date.month ).toBe( 10 ); // 1-imdex based month |
There was a problem hiding this comment.
Possible typo in the comment here.
| /* eslint-disable react-hooks/exhaustive-deps */ | ||
| // Rule disabled for compiled `paxDateRange` properties, instead of using the main object | ||
| // to ensure effect runs every time dates are changed. | ||
| useEffect( () => { | ||
| if ( displayMode === 'reporting' && paxAppRef?.current ) { | ||
| const { startDate, endDate } = paxDateRange; | ||
|
|
||
| paxAppRef.current.getServices().adsDateRangeService.update( { | ||
| startDate: formatPaxDate( startDate ), | ||
| endDate: formatPaxDate( endDate ), | ||
| } ); | ||
| } | ||
| }, [ | ||
| paxAppRef?.current, | ||
| registry, | ||
| displayMode, | ||
| `${ paxDateRange?.startDate }|${ paxDateRange?.endDate }`, | ||
| ] ); | ||
| /* eslint-enable react-hooks/exhaustive-deps */ |
There was a problem hiding this comment.
Have you tried destructuring paxDateRange above like so:
const { current, startDate, endDate } = useSelect( ... ) || {}
Could be cleaner and remove the need disabling the rule, although this may not fix the issue with updating correctly with the date updates.
There was a problem hiding this comment.
The idea is to source data outside the useEffect as dates are needed as dependency, and also avoid subscribing to the state from within hook. So we need to keep it there. Eslint rule is due to the not using variable directly but as parsed string.
There was a problem hiding this comment.
Won't creating the string as a new object cause this to re-run on every render? Either way, removing the hook warning about exhaustive dependencies means other errors could be made here, and ignoring these hook rules can lead to subtle bugs. I'd rather another way around this. 🤔
|
Thanks @benbowler PR updated |
|
Size Change: +2.6 kB (+0.18%) Total Size: 1.44 MB
ℹ️ View Unchanged
|
| /* eslint-disable react-hooks/exhaustive-deps */ | ||
| // Rule disabled for compiled `paxDateRange` properties, instead of using the main object | ||
| // to ensure effect runs every time dates are changed. | ||
| useEffect( () => { | ||
| if ( displayMode === 'reporting' && paxAppRef?.current ) { | ||
| const { startDate, endDate } = paxDateRange; | ||
|
|
||
| paxAppRef.current.getServices().adsDateRangeService.update( { | ||
| startDate: formatPaxDate( startDate ), | ||
| endDate: formatPaxDate( endDate ), | ||
| } ); | ||
| } | ||
| }, [ | ||
| paxAppRef?.current, | ||
| registry, | ||
| displayMode, | ||
| `${ paxDateRange?.startDate }|${ paxDateRange?.endDate }`, | ||
| ] ); | ||
| /* eslint-enable react-hooks/exhaustive-deps */ |
There was a problem hiding this comment.
Won't creating the string as a new object cause this to re-run on every render? Either way, removing the hook warning about exhaustive dependencies means other errors could be made here, and ignoring these hook rules can lead to subtle bugs. I'd rather another way around this. 🤔
| // eslint-disable-next-line require-await | ||
| get: async () => { |
There was a problem hiding this comment.
Does this actually need to be an async function? If PAX won't error, I don't see why we can't omit the async here 🤔
If we're going to leave an ESLint ignore in here though, which I'm fine with, we should add a comment explaining it.
| const partnerDateRange = | ||
| await services.partnerDateRangeService.get(); | ||
|
|
||
| /* eslint-disable sitekit/acronym-case */ |
There was a problem hiding this comment.
Why is this rule needed here? 🤔
…ite-kit-wp into enhancement/8687-pax-date-range.
| } ); | ||
| } | ||
| }, [ | ||
| paxAppRef?.current, |
There was a problem hiding this comment.
Using a ref as a dependency of a useEffect is an anti-pattern, because mutating the ref doesn't cause the effect run again.
Maybe we should make this paxAppRef a state value instead? It will trigger the component to render again, but I don't think that's actually an issue 🤔
There was a problem hiding this comment.
I've changed this to run the date update code after the app is initialised.
It was possible to re-render when the dates changes if they were destructured, as @benbowler mentioned.
There was a problem hiding this comment.
because mutating the ref doesn't cause the effect run again
I thought so, but actually it did re-trigger the hook once ref.current was set to the pax Instance, which is not re-run otherwise. As it changes value from undefined to an app instance, it is causing it to run again
There was a problem hiding this comment.
I've made some adjustments to the useEffect here so there aren't violations of the rules of hooks, as well as removing the use of a ref as a useEffect dependency (which doesn't work).
Works well now aside from the PAX app sometimes not updating its UI until there is a click/scroll event on the client, but I suspect that's a PAX app bug and not ours.
Summary
Addresses issue:
Relevant technical choices
PR Author Checklist
Do not alter or remove anything below. The following sections will be managed by moderators only.
Code Reviewer Checklist
Merge Reviewer Checklist