[docs-infra] Move Ads component to TypeScript #42842
Conversation
Netlify deploy previewhttps://deploy-preview-42842--material-ui.netlify.app/ Bundle size report |
| // The ratio of ads display reported to Google Analytics. Used to avoid an exceed on the Google Analytics quotas. | ||
| GA_ADS_DISPLAY_RATIO: 0.1, | ||
| }, |
There was a problem hiding this comment.
Not sure it's the best place to move the constants
There was a problem hiding this comment.
imo, ideally we move _app to the docs infra package, add the ads display ratio as a prop and provide it in a context.
For now I guess the approach works, though I don't like to depend on process.env in libraries, it's implicit API and bundlers may not always handle it.
There was a problem hiding this comment.
| }), | ||
| ...(adShape === 'inline2' && { | ||
| display: 'flex', | ||
| alignItems: 'flex-end', | ||
| }), | ||
| })} |
There was a problem hiding this comment.
I did not found usage of 'inline2' in material-ui, mui-x, or toolpad repo
| @@ -198,7 +199,7 @@ export default function Ad() { | |||
|
|
|||
| React.useEffect(() => { | |||
| // Avoid an exceed on the Google Analytics quotas. | |||
| if (Math.random() > GA_ADS_DISPLAY_RATIO || !eventLabel) { | |||
| if (Math.random() > ((process.env.GA_ADS_DISPLAY_RATIO ?? 0.1) as number) || !eventLabel) { | |||
There was a problem hiding this comment.
One thing to note as well is that Next.js actually puts a number in process.env.GA_ADS_DISPLAY_RATIO while in Node.js this would always be a string, so the types will always be incorrect.
Personally, I'd never pass anything to a Next.js env that is not a string and always parse it from a string on the client side. This would avoid the as number which circumvents the type checking on this line.
oliviertassinari
changed the title
| @@ -150,7 +151,7 @@ export default function Ad() { | |||
| const ad = React.useContext(AdContext); | |||
| const eventLabel = label ? `${label}-${ad.placement}-${adShape}` : null; | |||
|
|
|||
| const timerAdblock = React.useRef(); | |||
| const timerAdblock = React.useRef<NodeJS.Timeout>(); | |||
There was a problem hiding this comment.
The codebase uses:
| const timerAdblock = React.useRef<NodeJS.Timeout>(); | |
| const timerAdblock = React.useRef<ReturnType<typeof setTimeout>>(); |
There was a problem hiding this comment.
Modification done in #42944
| // The ratio of ads display reported to Google Analytics. Used to avoid an exceed on the Google Analytics quotas. | ||
| GA_ADS_DISPLAY_RATIO: 0.1, | ||
| }, |
There was a problem hiding this comment.
Yep, that's what I meant with #42842 (comment). Didn't know Next.js warned on this. Nice feature 🚀 . |
Moving all Ads related files to TS before moving them in
@mui/docs