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

[core] Fix package layout inconsistencies #45491

Merged
merged 10 commits into from
Mar 7, 2025

Conversation

DiegoAndai
Copy link
Member

@DiegoAndai DiegoAndai commented Mar 5, 2025

Fix two types issues discovered in mui/mui-x#16771:

  • The @mui/utils/types export couldn't be imported because it didn't follow the index.d.ts file pattern
  • Some system types exports were exporting a non-existent default types

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
@DiegoAndai DiegoAndai added the core Infrastructure work going on behind the scenes label Mar 5, 2025
@DiegoAndai DiegoAndai self-assigned this Mar 5, 2025
@mui-bot
Copy link

mui-bot commented Mar 5, 2025

Netlify deploy preview

https://deploy-preview-45491--material-ui.netlify.app/

Bundle size report

No bundle size changes (Toolpad)
No bundle size changes

Generated by 🚫 dangerJS against d87959d

@DiegoAndai DiegoAndai force-pushed the fix-utils-types-file branch from fb0e736 to d8ec7c5 Compare March 5, 2025 14:28
@DiegoAndai DiegoAndai requested a review from Janpot March 5, 2025 14:38
@DiegoAndai DiegoAndai marked this pull request as ready for review March 5, 2025 14:38
@Janpot
Copy link
Member

Janpot commented Mar 6, 2025

Fix: Move the types.ts file into the an index.ts file inside types folder

👌

Fix: Split affected index.ts files into index.js and index.d.ts and remove the default export from index.d.ts

I'm not sure I fully understand, src/borders.js has a default export. It just looks like the declaration file that types this js file is missing a type for that. Don't we just need to fix src/borders.d.ts?

index f6f3a6f9ba..e91d39b6ab 100644
--- a/packages/mui-system/src/borders/borders.d.ts
+++ b/packages/mui-system/src/borders/borders.d.ts
@@ -1,4 +1,4 @@
-import { PropsFor, SimpleStyleFunction, borders } from '../Box';
+import { PropsFor, SimpleStyleFunction, ComposedStyleFunction } from '../Box';
 
 export const border: SimpleStyleFunction<'border'>;
 export const borderTop: SimpleStyleFunction<'borderTop'>;
@@ -11,4 +11,27 @@ export const borderRightColor: SimpleStyleFunction<'borderRightColor'>;
 export const borderBottomColor: SimpleStyleFunction<'borderBottomColor'>;
 export const borderLeftColor: SimpleStyleFunction<'borderLeftColor'>;
 export const borderRadius: SimpleStyleFunction<'borderRadius'>;
+export const outline: SimpleStyleFunction<'outline'>;
+export const outlineColor: SimpleStyleFunction<'outlineColor'>;
+
+declare const borders: ComposedStyleFunction<
+  [
+    typeof border,
+    typeof borderTop,
+    typeof borderRight,
+    typeof borderBottom,
+    typeof borderLeft,
+    typeof borderColor,
+    typeof borderTopColor,
+    typeof borderRightColor,
+    typeof borderBottomColor,
+    typeof borderLeftColor,
+    typeof borderRadius,
+    typeof outline,
+    typeof outlineColor,
+  ]
+>;
+
 export type BordersProps = PropsFor<typeof borders>;
+
+export default borders;

It looks like it's missing multiple exports in the declaration file. (And why are we importing utility types from ../Box?)

We should probably just consider converting these to typescript instead of manually trying to keep declaration files in sync.

Janpot added 5 commits March 7, 2025 00:54
@@ -15,13 +15,15 @@ export interface StyleOptions<PropKey> {
themeKey?: string;
transform?: TransformFunction;
}
export function style<PropKey extends string, Theme extends object>(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the default export in the .js file.

@@ -48,6 +48,8 @@ export { default as GlobalStyles } from './GlobalStyles';
export type { GlobalStylesProps } from './GlobalStyles';

export * from './style';
export { default as style } from './style';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is how it's exported in the .js file

Janpot added 2 commits March 7, 2025 07:32
Copy link
Member

@Janpot Janpot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I updated the declaration files. We'll look into migrating to typescript separately.

@DiegoAndai
Copy link
Member Author

DiegoAndai commented Mar 7, 2025

I confirmed that the latest build in this PR fixes the issue 👌🏼

Thanks @Janpot!

@DiegoAndai DiegoAndai merged commit 2920fa3 into mui:master Mar 7, 2025
19 checks passed
@DiegoAndai DiegoAndai deleted the fix-utils-types-file branch March 7, 2025 11:37
mnajdova pushed a commit to mnajdova/material-ui that referenced this pull request Mar 11, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Infrastructure work going on behind the scenes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants