Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[lab] Drop usage of createStyles #24158

Merged
merged 8 commits into from Jan 4, 2021
Merged

Conversation

eps1lon
Copy link
Member

@eps1lon eps1lon commented Dec 28, 2020

Continuing #24132
Fixing symptoms of microsoft/TypeScript#42155

Should be reviewed while whitespace changes are hidden.

  1. Forbid createStyles (proof of concept: https://app.circleci.com/pipelines/github/mui-org/material-ui/30857/workflows/a296b04d-92ac-4037-b22f-6c67b1667390/jobs/207735)
  2. Drop createStyles from files that are sources of published declaration files
  3. Use explicit type annotations (MuiStyles + StyleRules for best autocomplete + error locations)

Diff of fixed inlined types (expires in 24h): https://www.diffchecker.com/SvdD2VuZ
Diff of generated declaration files: https://app.circleci.com/pipelines/github/mui-org/material-ui/31365/workflows/15c5b765-fa4b-4ff9-9ab5-c92f01c7cd60/jobs/209329/parallel-runs/0/steps/0-110

Part of #24112
Closes #24132

Will drop createStyles in a future PR for visibility. See #20012 (comment) Having worked with styles and types a bit more: createStyles is super helpful with regards to autocomplete. Will not drop.

@eps1lon eps1lon added bug 🐛 Something doesn't work typescript package: lab Specific to @mui/lab labels Dec 28, 2020
@mui-pr-bot
Copy link

mui-pr-bot commented Dec 28, 2020

Details of bundle changes

Generated by 🚫 dangerJS against 3c8b352

@eps1lon eps1lon force-pushed the fix/lab/inlined-types branch 2 times, most recently from d8d40f9 to d0d5637 Compare December 30, 2020 11:22
@eps1lon eps1lon marked this pull request as ready for review December 30, 2020 11:23
@eps1lon eps1lon changed the title [lab] Fix incorrect type imports in declaration files [lab] Drop usage of createStyles Dec 30, 2020
.eslintrc.js Outdated Show resolved Hide resolved
@eps1lon
Copy link
Member Author

eps1lon commented Dec 30, 2020

Want to get #24188 in first before we rebase and merge this PR.

@eps1lon eps1lon added the on hold There is a blocker, we need to wait label Dec 30, 2020
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Dec 31, 2020
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Dec 31, 2020
@eps1lon
Copy link
Member Author

eps1lon commented Dec 31, 2020

as const does not really produce what we want: https://app.circleci.com/pipelines/github/mui-org/material-ui/31205/workflows/8f67b930-081d-45fe-8eca-a658dbb7fe63/jobs/208591/parallel-runs/0/steps/0-110

The inline styles declaration is huge and not really what we want.

@eps1lon eps1lon marked this pull request as draft December 31, 2020 10:46
- not docoumented
- not tested
- not typed in /core
@eps1lon eps1lon removed the on hold There is a blocker, we need to wait label Jan 2, 2021
@eps1lon eps1lon marked this pull request as ready for review January 2, 2021 22:41
.eslintrc.js Outdated Show resolved Hide resolved
| 'pmButton'
| 'meridiemButtonSelected';

export const styles: MuiStyles<ClockClassKey> = (theme): StyleRules<ClockClassKey> => ({
Copy link
Member Author

Choose a reason for hiding this comment

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

This might seem like duplication but leads to better locations of type errors of classes or not implemented or not declared (compared to only using MuiStyles; just using Theme => StyleRules would not allow us to switch back to theme-less styles).

@eps1lon eps1lon merged commit e133d5c into mui:next Jan 4, 2021
@eps1lon eps1lon deleted the fix/lab/inlined-types branch January 4, 2021 13:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something doesn't work package: lab Specific to @mui/lab typescript
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants