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

[docs] Fix typechecking #15501

Merged
merged 9 commits into from
May 2, 2019
Merged

[docs] Fix typechecking #15501

merged 9 commits into from
May 2, 2019

Conversation

merceyz
Copy link
Member

@merceyz merceyz commented Apr 26, 2019

  • Added @material-ui/styles alias to tsconfig.json as docs failed to find it and tslint silently ignored the issue
  • Fixes incorrect typing for @material-ui/system/css

tsconfig.json Outdated Show resolved Hide resolved
@oliviertassinari oliviertassinari added the PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI label Apr 27, 2019
@eps1lon
Copy link
Member

eps1lon commented Apr 30, 2019

What does this accomplish? What files didn't we typecheck before?

@merceyz
Copy link
Member Author

merceyz commented Apr 30, 2019

It checks all files, compared to what happens now.
I don't know exactly what was not getting tested but it's clear the *.d.ts files in material-ui-styles wasn't getting checked properly, as it contains invalid imports.

@eps1lon
Copy link
Member

eps1lon commented May 1, 2019

I don't know exactly what was not getting tested but it's clear the *.d.ts files in material-ui-styles wasn't getting checked properly, as it contains invalid imports.

yarn workspace @material-ui/styles typescript did run on those files. I don't think we need another task.

Could you explain why these are invalid imports? Our demos are relying on them so it seems like they're fine.

@merceyz
Copy link
Member Author

merceyz commented May 1, 2019

Could you explain why these are invalid imports? Our demos are relying on them so it seems like they're fine.

If you check out this PR before I changed the imports and go to any of the demos and hover over the makeStyles function this is the result:
image
And classes:
image

From there go to the definition of makeStyles and scroll up to the imports, this is the result:
image

If you change the imports to be relative or add this to the root tsconfig.json it finds everything as it should

"@material-ui/styles": ["packages/material-ui-styles/src"],
"@material-ui/styles/*": ["packages/material-ui-styles/src/*"]

However, then you run into issues where createStyles isn't used, this issue is fixed in newer versions of TypeScript.
So the fix for these is either use createStyles, update typescript, or allow all properties to be strings
image

@eps1lon
Copy link
Member

eps1lon commented May 1, 2019

We can't verify if that isn't an issue with the editor. Those can e.g use different typescript versions.

Can you provide a clean repository that fails on a build step?

@merceyz
Copy link
Member Author

merceyz commented May 1, 2019

We can't verify if that isn't an issue with the editor. Those can e.g use different typescript versions.

VSCode is set to use the same version as the repo uses (3.2.2)

Can you provide a clean repository that fails on a build step?

No, it works perfectly fine other projects. It's only from the docs folder it's having issues.

@merceyz merceyz force-pushed the ci/typechecking branch from c292faf to 2c16577 Compare May 1, 2019 16:11
@merceyz merceyz force-pushed the ci/typechecking branch from f7ac3fd to ba0b2fc Compare May 1, 2019 16:22
@merceyz merceyz changed the title [CI] Added typechecking [docs] Fix typechecking May 1, 2019
@merceyz
Copy link
Member Author

merceyz commented May 1, 2019

@eps1lon Turns out tslint just ignored the import errors, I simplified this to just set the alias in docs/tsconfig.json

@mui-pr-bot
Copy link

mui-pr-bot commented May 1, 2019

No bundle size changes comparing e36d33a...960e17e

Generated by 🚫 dangerJS against 960e17e

@eps1lon eps1lon self-requested a review May 2, 2019 07:08
@eps1lon
Copy link
Member

eps1lon commented May 2, 2019

Well I have no idea what happened but it seems like our docs never type-checked properly anything related to @material-ui/styles. I don't know where typescript pulled the declarations for @material-ui/styles from or why it didn't report implicit any.

@merceyz
Copy link
Member Author

merceyz commented May 2, 2019

That's what I've been trying to get across xD

@eps1lon eps1lon mentioned this pull request May 2, 2019
@eps1lon
Copy link
Member

eps1lon commented May 2, 2019

@merceyz Yeah I'm super grateful for this. I had a lot of assumptions about our demos that were basically not true. I'm still not sure why this happened but at least we know what's wrong now.

I think we should rather use

--- a/tsconfig.json
+++ b/tsconfig.json
@@ -17,6 +17,8 @@
       "@material-ui/core/*": ["./material-ui/src/*"],
       "@material-ui/lab": ["./material-ui-lab/src"],
       "@material-ui/lab/*": ["./material-ui-lab/src/*"],
+      "@material-ui/styles": ["./material-ui-styles/src"],
+      "@material-ui/styles/*": ["./material-ui-styles/src/*"],
       "@material-ui/system": ["./material-ui-system/src"]
     }
   },

on the root tsconfig and add the noUnusedLocals later.

@merceyz
Copy link
Member Author

merceyz commented May 2, 2019

@eps1lon Agreed. Now the question is, what to do about the type inference issues, it's fixed in newer versions of typescript.

@merceyz
Copy link
Member Author

merceyz commented May 2, 2019

The error at material-ui/docs/src/pages/system/basics/JSS.tsx:14:6 is fixed by #15548

@eps1lon eps1lon removed the PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI label May 2, 2019
@eps1lon eps1lon merged commit f108bfd into mui:next May 2, 2019
@merceyz merceyz deleted the ci/typechecking branch May 2, 2019 14:42
@merceyz merceyz mentioned this pull request May 2, 2019
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants