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

TS errors with styled-engine-sc and skipLibCheck: false #34527

Closed
2 tasks done
jscheid opened this issue Sep 30, 2022 · 20 comments · Fixed by #39395
Closed
2 tasks done

TS errors with styled-engine-sc and skipLibCheck: false #34527

jscheid opened this issue Sep 30, 2022 · 20 comments · Fixed by #39395
Assignees
Labels
package: styled-engine-sc Specific to styled-components typescript

Comments

@jscheid
Copy link

jscheid commented Sep 30, 2022

Duplicates

  • I have searched the existing issues

Latest version

  • I have tested the latest version

Steps to reproduce 🕹

Steps:

git clone https://github.com/jscheid/mui-sc-lib-check
cd mui-sc-lib-check
yarn
yarn tsc

Current behavior 😯

TS compilation errors: https://gist.github.com/jscheid/c2debf024f6683153de705520e8f5ca6

Expected behavior 🤔

No errors.

Context 🔦

This is only an issue with skipLibCheck: false in tsconfig.json. When skipping lib checks, no errors appear.

It could be something I'm doing wrong? Specifically I'm not sure about including @types/styled-components, which I didn't see recommended anywhere. However, when I leave it out I'm getting a different set of errors.

Obviously this is easy to work around by setting skipLibCheck but I'd prefer to be able to enable these checks.

Your environment 🌎

See repo.

@jscheid jscheid added the status: waiting for maintainer These issues haven't been looked at yet by a maintainer label Sep 30, 2022
@mnajdova mnajdova added typescript package: styled-engine-sc Specific to styled-components labels Sep 30, 2022
@mnajdova mnajdova self-assigned this Sep 30, 2022
@mnajdova
Copy link
Member

Thanks for the report, I will take a look at it next week.

@vanyaxk
Copy link
Contributor

vanyaxk commented Oct 2, 2022

It looks like you're resolving @mui/styled-engine to @mui/styled-engine-sc, the latter does not export the interfaces at hand. The rest of the issues are eliminated by installing @emotion/react and @emotion/styled into the project

Hopefully this will be of help 🙂

@jscheid
Copy link
Author

jscheid commented Oct 2, 2022

@mnajdova thanks!

@vanyaxk thanks but this issue is specifically about typings with @mui/styled-engine-sc as described here, sorry if I haven't made that clear enough.

@mnajdova
Copy link
Member

mnajdova commented Oct 5, 2022

@jscheid have you checked the example we have that uses styled-components with typescript - https://github.com/mui/material-ui/tree/master/examples/create-react-app-with-styled-components-typescript?

To answer your question from above, yes you do need to have @types/styled-components. Can you check if there are differences in your config and the example project?

@mnajdova mnajdova added status: waiting for author Issue with insufficient information and removed status: waiting for maintainer These issues haven't been looked at yet by a maintainer labels Oct 5, 2022
@jscheid
Copy link
Author

jscheid commented Oct 5, 2022

@mnajdova the difference appears to be that styled-components-sc is swapped in via the bundler and not via Yarn, therefore tsc doesn't see the change.

Thanks for clarifying re: @types/styled-components.

@github-actions github-actions bot removed the status: waiting for author Issue with insufficient information label Oct 5, 2022
@mnajdova
Copy link
Member

I assume the issue was fixed then. I am closing.

@jscheid
Copy link
Author

jscheid commented Oct 14, 2022

Sorry but it's not fixed. If you follow these instructions and you have skipLibCheck: false, you're getting type checking errors.

@mnajdova
Copy link
Member

If it doesn’t happen here https://github.com/mui/material-ui/blob/master/examples/create-react-app-with-styled-components-typescript/tsconfig.json then there is nothing left I can do. Just follow the example. The instructions there are for scenarios where we don’t have example project

@jscheid
Copy link
Author

jscheid commented Oct 14, 2022

Your documentation offers multiple different methods of installing styled-engine-sc:

  1. with Yarn
  2. with Webpack
  3. with Next.js

The example that you refer to only shows that it works with the Webpack method.

Is the Yarn method still supported? If so, there is still a bug here because when using that method you're getting typing errors. If it's no longer supported, it should probably be removed from the documentation.

@trystan2k
Copy link

trystan2k commented Jan 15, 2023

If it doesn’t happen here https://github.com/mui/material-ui/blob/master/examples/create-react-app-with-styled-components-typescript/tsconfig.json then there is nothing left I can do. Just follow the example. The instructions there are for scenarios where we don’t have example project

@mnajdova I just downloaded the example you mentioned, did npm install and npm run tsc and got the same errors I am getting in my project (and the same that @jscheid reported):

image

Any suggestion on how to solve this (having skipLibCheck = true) ?

@jscheid
Copy link
Author

jscheid commented Jan 16, 2023

Oh yeah, that's a good repro... not sure how I missed that. Thanks @trystan2k.

curl https://codeload.github.com/mui/material-ui/tar.gz/master | tar -xz --strip=2 material-ui-master/examples/create-react-app-with-styled-components-typescript
cd create-react-app-with-styled-components-typescript
npm install
npm run tsc

@Gotos
Copy link

Gotos commented Sep 27, 2023

The same error occurs when running npx tsc --noEmit (or npm run tsc) in https://github.com/mui/material-ui/tree/master/examples/material-ui-cra-styled-components-ts - I'm not sure if that is the same example, the old links posted here don't seem to be working anymore.

But this means this error is still occurring in an official demo project. I'm not sure why this is closed to be honest. Am I missing something? (Genuine question, I don't mean to be sarcastic or snarky or anything.)
I'm kinda surprised this issue was pretty hard for me to find, I would have expected more traction on this issue considering it's so simple to reproduce and appears to happen in the official demo. So I'm really not convinced I'm not just missing something essential here.

For now, we are using the skipLibCheck = true, but if there are other ways to mitigate this issue, I'd love to hear about them.

@mnajdova
Copy link
Member

I am reopening and opening a pull request. Seems like we missed some types in this package that existed in the @mui/styled-engine. We also had some issues with mismatch of the generic types.

@mnajdova
Copy link
Member

I've setup this repository to test the changes with the last commit from #39395. Note, that it depends on v6 (that uses styled-components v6). If needed we can backport these changes to the v5 as well.

I can't spot any issues on the testing repository and the PR's CI is green, so everything should be fine. Feel free to test out the repo and report back if you find any issues.

@Gotos
Copy link

Gotos commented Oct 12, 2023

Our codebase is currently in the process of being migrated to V5, so a backport would be highly appreciated indeed.

Thanks for your support!

@mnajdova
Copy link
Member

Our codebase is currently in the process of being migrated to V5

Do you mean Mateiral UI v5? I mentioned this for the styled-components version. You can use @mui/material v5 with @mui/styled-engine-sc v6 & styled-components v6. These are compatible, this is what the repository I shared uses.

@Gotos
Copy link

Gotos commented Oct 13, 2023

You are indeed right. However we have a few dependencies still relying on styled-components v5 that seem incompatible with v6 due to typing issues. Granted, those dependencies look like they are all our own internal libraries and I'm looking into updating them right now, but if the backport is simple, I'd still appreciate it a lot! :)

@mnajdova
Copy link
Member

if the backport is simple, I'd still appreciate it a lot! :)

It wouldn't be easy, we'll need to keep two branches only for this dependency, ultimately I would avoid this if possible.

@Gotos
Copy link

Gotos commented Oct 13, 2023

Okay. I'll discuss with my co-workers on Monday but after looking into said libraries, I'm optimistic that we can use styled-components/styled-engine-sc v6 and won't need a backport.

@Gotos
Copy link

Gotos commented Oct 16, 2023

After consulting with my co-workers, we are now in the process of migrating to styled-components and styled-engine-sc v6, so no backport needed for us.

Thanks again for your support!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package: styled-engine-sc Specific to styled-components typescript
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants