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

[icons] Consider as written in TypeScript #22108

Closed
wants to merge 2 commits into from

Conversation

oliviertassinari
Copy link
Member

@oliviertassinari oliviertassinari commented Aug 8, 2020

A continuation on #15984 (comment). It seems that migrating the icons from .js to a .tsx extension wouldn't make sense considering that we generate all these files as well as their .d.ts definitions. The current approach is more efficient. However, I believe we can consider these files are written in TypeScript.

Before

$ github-linguist
94.80%  JavaScript
5.19%   TypeScript
0.01%   HTML
0.00%   Shell

After

$ github-linguist
54.48%  TypeScript
45.50%  JavaScript
0.01%   HTML
0.00%   Shell

I have considered using linguist-generated until I saw:

be ignored for the repository's language statistics and hidden by default in diffs

https://docs.github.com/en/github/administering-a-repository/customizing-how-changed-files-appear-on-github

@oliviertassinari oliviertassinari added typescript package: icons Specific to @mui/icons labels Aug 8, 2020
@mui-pr-bot
Copy link

No bundle size changes comparing 993a623...3318b7a

Generated by 🚫 dangerJS against 3318b7a

@eps1lon
Copy link
Member

eps1lon commented Aug 8, 2020

I mean it's not TS so it's not TS.

@oliviertassinari
Copy link
Member Author

oliviertassinari commented Aug 8, 2020

I mean it's not TS so it's not TS.

@eps1lon I can't disagree, JS is not TS 😆. Here is the underlying problem I'm going after:

  • Short term: the reported JavaScript vs TypeScript ratio doesn't seem to draw an accurate representation of the usage of TypeScript in the codebase: 5% of TypeScript sources?!
  • Long term: we are progressively moving toward a codebase fully written in TypeScript, once we will have fully migrated the components, we would still have GitHub reports a majority of JavaScript files which will be still misleading for new users. Why? because the icons account for half the files in the repository.

The only alternative I can think of is to generate .tsx rather than .js files in the icons script. However, this seems completely artificial, as we would skip tslint on these files to save time (no need to lint the, it's always the same pattern).

@oliviertassinari
Copy link
Member Author

oliviertassinari commented Aug 8, 2020

This change assumes that people look at the language states and that it has an influence in their decisions for picking a dependency, that when they see 5%, they will search for "TypeScript" in the issue history to better understand the state of it. They will land on #15984 and blindly upvote the issue without spending a lot of time looking closely at the coverage.

@oliviertassinari
Copy link
Member Author

has an influence in their decisions for picking a dependency

Motivation number 4 in or survey results: https://material-ui.com/blog/2020-developer-survey-results/#6-what-are-your-key-criteria-when-choosing-a-ui-library

@oliviertassinari
Copy link
Member Author

Question 6 in the survey is about user acquisition, which is not to be confused with current users that experience a lot less TypeScript issues in the survey of 2020 than 2019 :)

@eps1lon
Copy link
Member

eps1lon commented Aug 8, 2020

But the stats are wrong either way. At least previously we didn't intentionally manipulate them.

@oliviertassinari
Copy link
Member Author

Moving to #22188

@oliviertassinari oliviertassinari deleted the icons-typescript branch August 29, 2020 11:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package: icons Specific to @mui/icons typescript
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants