-
-
Notifications
You must be signed in to change notification settings - Fork 32.3k
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
[material-ui][mui-system] Add support for version runtime checks #43190
Merged
Merged
Changes from all commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
059f3af
Add support for version const
DiegoAndai c6542ff
Rename releaseConst script to updateVersionConst
DiegoAndai 744c5a6
Refactor to add version at compilation
DiegoAndai c1e460d
Add parsed exports
DiegoAndai a0c31bd
Remove redundant export from index
DiegoAndai e06ebc8
Refactor to parse values at build time
DiegoAndai File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,8 @@ | ||
export const version = process.env.MUI_VERSION; | ||
export const major = Number(process.env.MUI_MAJOR_VERSION); | ||
export const minor = Number(process.env.MUI_MINOR_VERSION); | ||
export const patch = Number(process.env.MUI_PATCH_VERSION); | ||
export const preReleaseLabel = process.env.MUI_PRERELEASE_LABEL || null; | ||
export const preReleaseNumber = Number(process.env.MUI_PRERELEASE_NUMBER) || null; | ||
|
||
export default version; |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,8 @@ | ||
export const version = process.env.MUI_VERSION; | ||
export const major = Number(process.env.MUI_MAJOR_VERSION); | ||
export const minor = Number(process.env.MUI_MINOR_VERSION); | ||
export const patch = Number(process.env.MUI_PATCH_VERSION); | ||
export const preReleaseLabel = process.env.MUI_PRERELEASE_LABEL || null; | ||
export const preReleaseNumber = Number(process.env.MUI_PRERELEASE_NUMBER) || null; | ||
|
||
export default version; |
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Oops, something went wrong.
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder, what if we were to only expose the version? Less bundle size:
e.g. React https://github.com/facebook/react/blob/main/packages/shared/ReactVersion.js
or jQuery, I'm not aware they export more.
But fair enough #43190 (comment), then just
in this case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think these five (or two) consts make much difference in bundle size. Unpacking them is useful (#43190 (comment)) and having the pre release consts keeps it consistent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I get the value of version, major, minor, patch but I don't understand
preReleaseLabel
,preReleaseNumber
.I do believe that we should care it, bundle size is a fight of every single PR, if done systematically, it starts to add-up, and in small instances I think it sends the right signal to the community.
https://unpkg.com/browse/@mui/material@6.0.0/version/index.js shows:
So 0.1 kB gzipped saving potential 😄? I would be curious to play https://goober.rocks/the-great-shave-off for Material UI
I love Elon's point in https://www.youtube.com/watch?v=tRsxLLghL1k&t=574s: unless you have to add back 10% of what you remove, you are not removing enough.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bundle-size-wise I don't really mind either way. I expect these to have 0% impact on end-user bundles. These will be fully tree-shaken by minifiers if unused.
But I agree it's not clean (
isNaN(Number(undefined))
...). In hindsight I'd say just aprerelease: string | undefined
export makes more sense to me.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this makes sense 👍🏼
That was my understanding as well.
So what option makes the most sense to you @oliviertassinari @Janpot?:
preReleaseLabel
andpreReleaseNumber
withpreRelease: string | undefined
preReleaseLabel
andpreReleaseNumber
Both could be considered breaking changes, but "not really" as those were only available in pre-release versions. If we want to be extra conservative, we can deprecate and remove them from the next major.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Janpot True 🙃, though one could argue that if nobody imports them then we don't need them.
Happy either way 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From an API point of view changing it
prerelease: string | undefined
makes the most sense to me. But I don't have a strong opinion, I only see use for the major so far.From a semver point of view it depends on how pedantic we want to be about this being a breaking change, but I think we can still kill it (edit:
preReleaseLabel
andpreReleaseNumber
in favor forprerelease
I mean).In the end we decided we'd just don't support css vars themes in toolpad when you're on v5 (which we initially did support), so we don't need this anymore at the moment. It keeps the door open for feature detection in the future (yagni?). I really have no strong opinion on this one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can see the purpose in having this long term: third party libraries who want to implement backward support between our different majors might appreciate that we have it. We could have it in, and see if this is used long term.
I guess, I'm a bit wired like this 😁: https://youtu.be/tRsxLLghL1k?si=sRhbVRHVLRR83pMX&t=574. "if you are not adding back at least 10% of what you are removing, you know you are not removing enough", why I started this thread.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well essentially the 10% that was removed was the
CssVarsProvider
as a separate theme provider. On toolpad side we couldn't know whether theThemeProvider
from our peer dependency supported css vars themes or not. (In 5 it doesn't and we wanted to fall back toCssVarsProvider
in that case, as we were already doing). In the end we decided to break backwards compatibilty so the problem went away altogether. but at that timeversion
was already added to @mui/material. I'm totally fine removing it again. But we saw a clear need at some point that is not unthinkable to return in v7.Essentially we took (the proverbial) 10% out and had to 0.1% back in. It's even better 😄