[code-infra] Inject display name #44067
Draft
Conversation
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
Netlify deploy previewhttps://deploy-preview-44067--material-ui.netlify.app/ @material-ui/core: parsed: +0.18% , gzip: +0.13% Bundle size reportDetails of bundle changes (Toolpad) |
packages-internal/scripts/typescript-to-proptypes/src/injectPropTypesInFile.ts
Outdated
Show resolved
Hide resolved
packages-internal/scripts/typescript-to-proptypes/src/injectPropTypesInFile.ts
Outdated
Show resolved
Hide resolved
| @@ -167,4 +167,6 @@ Button.propTypes /* remove-proptypes */ = { | |||
| to: PropTypes.string, | |||
| } as any; | |||
|
|
|||
| Button.displayName = 'Button'; | |||
There was a problem hiding this comment.
What about?
Suggested change
| Button.displayName = 'Button'; | |
| if(process.env.NODE_ENV !== 'production') { | |
| Button.displayName = 'Button'; | |
| } |
this would mean:
- be sure there is no regressions
- give developers developer confidence that this isn't going to bloat their bundle
- In the code ejection story, we could automatically wrap this, but I would expect people to also copy and paste the source too
There was a problem hiding this comment.
I thought about it but went for the current solution because
- Parity with the proptypes generation. Both work the exact same way: upsert code => wrap in build step. If you understand one, you also understand the other. If we add in the conditional for displayName, then I think we should do the same for
propTypesand remove the babel plugin. - It's more straightforward and robust for the detection logic if it consists of a single top-level statement statement.
- It prevents people from getting tempted adding more things in the coditional. Which in turn would either break the detection logic, or make it much more complex.
There was a problem hiding this comment.
Parity with the proptypes generation
Ah right ok, I forgot, I thought we were doing the opposite. Makes sense then 👍
…opTypesInFile.ts Co-authored-by: Olivier Tassinari <[email protected]> Signed-off-by: Jan Potoms <[email protected]>
…opTypesInFile.ts Co-authored-by: Olivier Tassinari <[email protected]> Signed-off-by: Jan Potoms <[email protected]>
This was referenced Oct 16, 2024
|
Should this be a babel plugin instead? Do we need to commit these changes? 🤔 |
github-actions
bot
added
PR: out-of-date
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Implements mui/mui-public#209 to stabilise tests and improve DX
Individual commits for easier review: