[zero][runtime] Implement typings for public runtime API #39215
Conversation
brijeshb42
commented
Sep 29, 2023
•
brijeshb42
commented
- I have followed (at least) the PR section of the contributing guide.
Netlify deploy previewhttps://deploy-preview-39215--material-ui.netlify.app/ Bundle size report |
brijeshb42
force-pushed
the
zero-typescript-support
branch
4 times, most recently
from
97e2bc6
to
cb29cac
Compare
brijeshb42
force-pushed
the
zero-typescript-support
branch
from
cb29cac
to
c41e942
Compare
apps/zero-runtime-next-app/src/components/Slider/ZeroSlider.tsx
Outdated
Show resolved
Hide resolved
| @@ -286,8 +301,8 @@ const SliderTrack = styled('span', { | |||
| color: 'secondary', | |||
| }, | |||
| style: { | |||
| backgroundColor: theme.vars ? theme.vars.palette.Slider.secondaryTrack : undefined, | |||
| borderColor: theme.vars ? theme.vars.palette.Slider.secondaryTrack : undefined, | |||
| backgroundColor: (theme.vars ?? theme).palette.Slider?.secondaryTrack, | |||
There was a problem hiding this comment.
We should be able to not use this many conditional chaining if the types of the theme are correctly defined, no?
There was a problem hiding this comment.
Yes. That comes from the user. In this case, I specified vars as {vars?: ThemeVars} that's why the conditionals.
apps/zero-runtime-next-app/src/components/Slider/ZeroSlider.tsx
Outdated
Show resolved
Hide resolved
| @@ -1,5 +1,8 @@ | |||
| { | |||
| "env": { | |||
| "jest": true | |||
| }, | |||
| "rules": { | |||
| "react/react-in-jsx-scope": "off" | |||
There was a problem hiding this comment.
Why block this? We can import React where it makes sense.
There was a problem hiding this comment.
We should rather be consistent in the repo.
There was a problem hiding this comment.
It's a vite app with jsx as auto. So it's not needed to import React if you are only using JSX
There was a problem hiding this comment.
Hi @mnajdova Could you look into the PR again? I've implemented majority of the requested changes.
brijeshb42
force-pushed
the
zero-typescript-support
branch
3 times, most recently
from
ae5a7b7
to
07fe5fc
Compare
github-actions
bot
added
the
PR: out-of-date
brijeshb42
force-pushed
the
zero-typescript-support
branch
from
07fe5fc
to
a00e583
Compare
github-actions
bot
removed
the
PR: out-of-date
brijeshb42
force-pushed
the
zero-typescript-support
branch
6 times, most recently
from
9cf6686
to
d15e563
Compare
| @@ -748,6 +756,7 @@ const Slider = React.forwardRef(function Slider(props, ref) { | |||
| }, | |||
| ownerState: { | |||
| ...ownerState, | |||
| // @ts-expect-error | |||
There was a problem hiding this comment.
I guess this is consumer-facing side, not related to zero-runtime.
github-actions
bot
added
the
PR: out-of-date
| }, | ||
| { | ||
| props: { | ||
| isBlue: true, |
There was a problem hiding this comment.
Shouldn't this throw, as isBlue is not part of the props interface?
There was a problem hiding this comment.
It does not explicitly forbid passing unknown props but'll provide autocompletion for known props.
I verified that it works the same way with @mui/system.
There was a problem hiding this comment.
In that case, we should fix it in the @mui/system, too, and not replicate the wrong behavior (at least in my opinion is wrong). Is there a limitation that it was implemented like this? Can we fix it?
Considering this will be the API that will be used mostly about styling the components, we need to make sure it is strictly typed. Otherwise, image the callbacks not having a strict props type, it can cause so many issues.
I am approving we do a follow-up, to allow @siriwatknp to use the types we have so far.
There was a problem hiding this comment.
I am still exploring a way to fix it. There's one more feature to be added (automatic prop suggestion when as prop is provided with a different element. Will try to add both the fixes together.
for FunctionExpression
brijeshb42
force-pushed
the
zero-typescript-support
branch
from
d15e563
to
deb1101
Compare
github-actions
bot
removed
the
PR: out-of-date
