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] Add exports field to packages #41596

Closed
wants to merge 23 commits into from
Closed
Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
1646c6f
Initial commit from POC changes
DiegoAndai Mar 21, 2024
365aaea
Fix @app/next-app build
DiegoAndai Mar 21, 2024
a8a9c75
Refactor exports field flag
DiegoAndai Mar 21, 2024
330c04b
Update mui-material/src/styles/index.d.ts
DiegoAndai Mar 21, 2024
f3f6a46
Refactor build structure check and exports field computation
DiegoAndai Mar 22, 2024
bf84a60
Add `@mui/types` exports field
DiegoAndai Mar 22, 2024
91383ac
Breaking changes guide
DiegoAndai Mar 27, 2024
222eef3
Merge branch 'next' into add-exports-field
DiegoAndai Apr 10, 2024
bee0004
Refactor to add compatibility mode with previous configuration
DiegoAndai Apr 10, 2024
9b37c2a
Add modern export to exports field
DiegoAndai Apr 11, 2024
d7d133d
pnpm dedupe
DiegoAndai Apr 12, 2024
a28200d
pnpm prettier
DiegoAndai Apr 12, 2024
9fe59d8
Update broken link
DiegoAndai Apr 12, 2024
74cb37c
Exclude codemod and docs packages from exports format
DiegoAndai Apr 12, 2024
6bda919
pnpm dedupe
DiegoAndai Apr 12, 2024
6bad76d
Merge branch 'next' into add-exports-field
DiegoAndai Apr 15, 2024
5d0050b
pnpm install / pnpm dedupe
DiegoAndai Apr 15, 2024
35a501e
Merge branch 'next' into add-exports-field
DiegoAndai Apr 15, 2024
90a3325
Change "modern" custom export to "mui-moder"
DiegoAndai Apr 16, 2024
b94b4a6
Make exports field opt-in instead of opt-out
DiegoAndai Apr 16, 2024
6964f8b
Merge branch 'next' into add-exports-field
DiegoAndai Apr 16, 2024
7442410
Remove 'use client' directive from index files
DiegoAndai Apr 18, 2024
fb7a4ff
Exclude material-nextjs from exports format build
DiegoAndai Apr 18, 2024
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion apps/pigment-css-vite-app/src/Slider/ZeroSlider.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ import {
import { isHostComponent, useSlotProps } from '@mui/base/utils';
import { styled } from '@pigment-css/react';
import { capitalize } from '@mui/material/utils';
import SliderValueLabel from '@mui/material/Slider/SliderValueLabel';
import { SliderValueLabel } from '@mui/material/Slider';
import { useSlider, valueToPercent } from '@mui/base/useSlider';
import { alpha, lighten, darken } from '@mui/system/colorManipulator';
import type { Theme } from '@mui/material/styles';
Expand Down
4 changes: 4 additions & 0 deletions babel.config.js
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,10 @@ module.exports = function getBabelConfig(api) {
]);
}

if (process.env.MUI_ADD_IMPORT_EXTENSIONS === 'true') {
Copy link
Member

Choose a reason for hiding this comment

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

Is there a reason to not always add the extensions?

Copy link
Member Author

Choose a reason for hiding this comment

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

The regressions and rollup (umd) builds use this config file as well, and those break if we add the extensions.

When we remove the umd build, I could test if the issue with the regressions build is fixable, and we can remove this check. Does that make sense?

Copy link
Member

Choose a reason for hiding this comment

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

Ideally al tests use builds that are as close to the real build as possible

Copy link
Member Author

@DiegoAndai DiegoAndai Mar 22, 2024

Choose a reason for hiding this comment

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

That makes sense, I'll look into adapting the regressions builds as well 👍🏼

Copy link
Member Author

Choose a reason for hiding this comment

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

Hey @Janpot, I investigated the issue a bit. The regressions build uses babel.config.js to compile the packages as well as other files, like the docs data files and the regressions tests files. It also points to the src of the packages instead of the build (see the regressions webpack config and the base webpack config).

I was able to use the packages builds instead of the src code by doing

diff --git a/test/regressions/webpack.config.js b/test/regressions/webpack.config.js
index b4472eedd1..c9d02bb705 100644
--- a/test/regressions/webpack.config.js
+++ b/test/regressions/webpack.config.js
@@ -64,6 +64,20 @@ module.exports = {
       // Exclude polyfill and treat 'zlib' as an empty module since it is not required. next -> gzip-size relies on it.
       zlib: false,
     },
+    alias: {
+      ...webpackBaseConfig.resolve.alias,
+      '@mui/material': path.resolve(__dirname, '../../packages/mui-material/build'),
+      '@mui/icons-material': path.resolve(__dirname, '../../packages/mui-icons-material/build/esm'),
+      '@mui/lab': path.resolve(__dirname, '../../packages/mui-lab/build'),
+      '@mui/styled-engine': path.resolve(__dirname, '../../packages/mui-styled-engine/build'),
+      '@mui/styled-engine-sc': path.resolve(__dirname, '../../packages/mui-styled-engine-sc/build'),
+      '@mui/styles': path.resolve(__dirname, '../../packages/mui-styles/build'),
+      '@mui/system': path.resolve(__dirname, '../../packages/mui-system/build'),
+      '@mui/private-theming': path.resolve(__dirname, '../../packages/mui-private-theming/build'),
+      '@mui/base': path.resolve(__dirname, '../../packages/mui-base/build'),
+      '@mui/utils': path.resolve(__dirname, '../../packages/mui-utils/build'),
+      '@mui/joy': path.resolve(__dirname, '../../packages/mui-joy/build'),
+    },
   },

But even after doing that I still had to filter out some files and not add the file extensions to them:

diff --git a/babel.config.js b/babel.config.js
index 12d09a03c2..2d3ba4a891 100644
--- a/babel.config.js
+++ b/babel.config.js
@@ -103,10 +103,6 @@ module.exports = function getBabelConfig(api) {
     ]);
   }
 
-  if (process.env.MUI_ADD_IMPORT_EXTENSIONS === 'true') {
-    plugins.push(['babel-plugin-add-import-extension', { extension: useESModules ? 'mjs' : 'js' }]);
-  }
-
   return {
     assumptions: {
       noDocumentAll: true,
@@ -119,6 +115,12 @@ module.exports = function getBabelConfig(api) {
         exclude: /\.test\.(js|ts|tsx)$/,
         plugins: ['@babel/plugin-transform-react-constant-elements'],
       },
+      {
+        exclude: /(\/test\/regressions|packages\/mui-docs|docs)/,
+        plugins: [
+          ['babel-plugin-add-import-extension', { extension: useESModules ? 'mjs' : 'js' }],
+        ],
+      },
     ],
     env: {
       coverage: {

This is because the files in those folders do not have extensions, as we're not adding them. We could add them, but I think this is outside of this PR's scope.

In conclusion, I would do

  1. Maintain the MUI_ADD_IMPORT_EXTENSIONS flag for this PR, adding the extensions only when building using build.mjs
  2. Remove the UMD build in a separate PR
  3. Create an issue to use the package builds instead of src for the regression tests. In that issue's PR, we could revisit removing MUI_ADD_IMPORT_EXTENSIONS and modifying the regression infrastructure accordingly.

What do you think?

Copy link
Member

Choose a reason for hiding this comment

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

Ok for me

plugins.push(['babel-plugin-add-import-extension', { extension: useESModules ? 'mjs' : 'js' }]);
}

return {
assumptions: {
noDocumentAll: true,
Expand Down
Original file line number Diff line number Diff line change
@@ -1,9 +1,9 @@
import * as React from 'react';
// import of a small, pure module in a private demo
// bundle size and module duplication is negligible
/* eslint-disable-next-line no-restricted-imports */
import { convertLength } from '@mui/material/styles/cssUtils';
import { createTheme, responsiveFontSizes } from '@mui/material/styles';
import {
createTheme,
responsiveFontSizes,
unstable_convertLength as convertLength,
} from '@mui/material/styles';
import Box from '@mui/material/Box';
import { LineChart } from '@mui/x-charts';

Expand Down
9 changes: 5 additions & 4 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -84,8 +84,9 @@
},
"dependencies": {
"@googleapis/sheets": "^5.0.5",
"@slack/bolt": "^3.17.1",
"@netlify/functions": "^2.6.0",
"@slack/bolt": "^3.17.1",
"babel-plugin-add-import-extension": "^1.6.0",
"execa": "^8.0.1",
"google-auth-library": "^9.7.0"
},
Expand All @@ -106,17 +107,17 @@
"@babel/preset-typescript": "^7.23.3",
"@babel/register": "^7.23.7",
"@mnajdova/enzyme-adapter-react-18": "^0.2.0",
"@mui/internal-docs-utils": "workspace:^",
"@mui/internal-scripts": "workspace:^",
"@mui-internal/api-docs-builder": "workspace:^",
"@mui-internal/api-docs-builder-core": "workspace:^",
"@mui-internal/test-utils": "workspace:^",
"@mui/internal-docs-utils": "workspace:^",
"@mui/internal-scripts": "workspace:^",
"@mui/joy": "workspace:*",
"@mui/material": "workspace:^",
"@mui/utils": "workspace:^",
"@pigment-css/react": "workspace:^",
"@next/eslint-plugin-next": "^14.1.3",
"@octokit/rest": "^20.0.2",
"@pigment-css/react": "workspace:^",
"@playwright/test": "1.42.1",
"@types/enzyme": "^3.10.18",
"@types/fs-extra": "^11.0.4",
Expand Down
2 changes: 1 addition & 1 deletion packages/mui-core-downloads-tracker/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@
},
"scripts": {
"build": "mkdir build && pnpm build:copy-files",
"build:copy-files": "node ../../scripts/copyFiles.mjs",
"build:copy-files": "node ../../scripts/copyFiles.mjs --skipExportsField",
DiegoAndai marked this conversation as resolved.
Show resolved Hide resolved
"prebuild": "rimraf build",
"release": "pnpm build && pnpm publish"
},
Expand Down
2 changes: 1 addition & 1 deletion packages/mui-icons-material/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@
"build:modern": "echo 'Skip modern build'",
"build:node": "node ../../scripts/build.mjs node --largeFiles --outDir lib",
"build:stable": "node ../../scripts/build.mjs stable --largeFiles --outDir lib",
"build:copy-files": "node ../../scripts/copyFiles.mjs",
"build:copy-files": "node ../../scripts/copyFiles.mjs --skipExportsField",
"build:typings": "node ./scripts/create-typings.mjs",
"prebuild": "rimraf build",
"release": "pnpm build && pnpm publish",
Expand Down
12 changes: 10 additions & 2 deletions packages/mui-material/src/styles/index.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,11 @@ export { ComponentsProps, ComponentsPropsList } from './props';
export { ComponentsVariants } from './variants';
export { ComponentsOverrides, ComponentNameToClassKey } from './overrides';
export { Components } from './components';
export { getUnit as unstable_getUnit, toUnitless as unstable_toUnitless } from './cssUtils';
export {
getUnit as unstable_getUnit,
toUnitless as unstable_toUnitless,
convertLength as unstable_convertLength,
} from './cssUtils';

export type ClassNameMap<ClassKey extends string = string> = Record<ClassKey, string>;

Expand All @@ -97,7 +101,11 @@ export { default as makeStyles } from './makeStyles';
export { default as withStyles } from './withStyles';
export { default as withTheme } from './withTheme';

export * from './CssVarsProvider';
export {
useColorScheme,
getInitColorSchemeScript,
Experimental_CssVarsProvider,
} from './CssVarsProvider';

export { default as experimental_extendTheme } from './experimental_extendTheme';
export type {
Expand Down
12 changes: 10 additions & 2 deletions packages/mui-material/src/styles/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,11 @@ export function experimental_sx() {
export { default as createTheme, createMuiTheme } from './createTheme';
export { default as unstable_createMuiStrictModeTheme } from './createMuiStrictModeTheme';
export { default as createStyles } from './createStyles';
export { getUnit as unstable_getUnit, toUnitless as unstable_toUnitless } from './cssUtils';
export {
getUnit as unstable_getUnit,
toUnitless as unstable_toUnitless,
convertLength as unstable_convertLength,
} from './cssUtils';
export { default as responsiveFontSizes } from './responsiveFontSizes';
export { duration, easing } from './createTransitions';
export { default as useTheme } from './useTheme';
Expand All @@ -44,7 +48,11 @@ export { default as makeStyles } from './makeStyles';
export { default as withStyles } from './withStyles';
export { default as withTheme } from './withTheme';

export * from './CssVarsProvider';
export {
useColorScheme,
getInitColorSchemeScript,
Experimental_CssVarsProvider,
} from './CssVarsProvider';
export { default as experimental_extendTheme } from './experimental_extendTheme';
export { default as getOverlayAlpha } from './getOverlayAlpha';
export { default as shouldSkipGeneratingVar } from './shouldSkipGeneratingVar';
Expand Down
2 changes: 1 addition & 1 deletion packages/mui-types/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@
"homepage": "https://github.com/mui/material-ui/tree/master/packages/mui-types",
"scripts": {
"build": "mkdir build && cpy index.d.ts build/ && cpy OverridableComponentAugmentation.ts build/ && pnpm build:copy-files",
"build:copy-files": "node ../../scripts/copyFiles.mjs",
"build:copy-files": "node ../../scripts/copyFiles.mjs --skipExportsField",
DiegoAndai marked this conversation as resolved.
Show resolved Hide resolved
"prebuild": "rimraf build",
"release": "pnpm build && pnpm publish",
"test": "echo 'No runtime test. Type tests are run with the `typescript` script.'",
Expand Down
12 changes: 12 additions & 0 deletions pnpm-lock.yaml

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 5 additions & 0 deletions scripts/build.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ async function run(argv) {
NODE_ENV: 'production',
BABEL_ENV: bundle,
MUI_BUILD_VERBOSE: verbose,
MUI_ADD_IMPORT_EXTENSIONS: true,
};
const babelConfigPath = path.resolve(getWorkspaceRoot(), 'babel.config.js');
const srcDir = path.resolve('./src');
Expand Down Expand Up @@ -69,6 +70,8 @@ async function run(argv) {
}[bundle],
);

const outExtension = bundle === 'node' ? '.js' : '.mjs';

const babelArgs = [
'--config-file',
babelConfigPath,
Expand All @@ -77,6 +80,8 @@ async function run(argv) {
srcDir,
'--out-dir',
outDir,
'--out-file-extension',
outExtension,
'--ignore',
// Need to put these patterns in quotes otherwise they might be evaluated by the used terminal.
`"${ignore.join('","')}"`,
Expand Down
38 changes: 31 additions & 7 deletions scripts/copyFiles.mjs
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
/* eslint-disable no-console */
import path from 'path';
import yargs from 'yargs';
import {
createModulePackages,
createPackageFile,
Expand All @@ -23,9 +24,9 @@ async function addLicense(packageData) {
`;
await Promise.all(
[
'./index.js',
'./legacy/index.js',
'./modern/index.js',
'./index.mjs',
'./legacy/index.mjs',
'./modern/index.mjs',
'./node/index.js',
'./umd/material-ui.development.js',
'./umd/material-ui.production.min.js',
Expand All @@ -43,13 +44,14 @@ async function addLicense(packageData) {
);
}

async function run() {
const extraFiles = process.argv.slice(2);
async function run(argv) {
const { extraFiles, skipExportsField } = argv;

try {
// TypeScript
await typescriptCopy({ from: srcPath, to: buildPath });

const packageData = await createPackageFile();
const packageData = await createPackageFile(skipExportsField);

await Promise.all(
['./README.md', '../../CHANGELOG.md', '../../LICENSE', ...extraFiles].map(async (file) => {
Expand All @@ -67,4 +69,26 @@ async function run() {
}
}

run();
yargs(process.argv.slice(2))
.command({
command: '$0 [extraFiles..]',
description: 'copy files',
builder: (command) => {
return command
.positional('extraFiles', {
type: 'array',
default: [],
})
.option('skipExportsField', {
Copy link
Member Author

Choose a reason for hiding this comment

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

This might be a temporal flag until we close #35233, which we're doing next. Depending on how the icons package is handled it might or might not be necessary.

type: 'boolean',
default: false,
describe:
'Set to `true` if you wish to skip adding the exports field. Adding the exports field is only supported for top level ESM packages.',
});
},
handler: run,
})
.help()
.strict(true)
.version(false)
.parse();
49 changes: 43 additions & 6 deletions scripts/copyFilesUtils.mjs
Copy link
Member Author

Choose a reason for hiding this comment

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

@Janpot @cherniavskii I'm wondering how these changes will impact X's script: https://github.com/mui/mui-x/blob/next/scripts/copyFiles.mjs.

  • We'll have to do createPackageFile(true) to skip the exports field
  • For the module field .js => .mjs change, should I add a flag as well?

Copy link
Member

Choose a reason for hiding this comment

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

hmm, X will have to adopt these changes at well, but likely not before the next major. in the meantime I think we'll need a compatibility mode indeed.

Copy link
Member Author

Choose a reason for hiding this comment

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

I will implement it.

Original file line number Diff line number Diff line change
Expand Up @@ -45,8 +45,8 @@ export async function createModulePackages({ from, to }) {
const packageJson = {
sideEffects: false,
module: topLevelPathImportsAreCommonJSModules
? path.posix.join('../esm', directoryPackage, 'index.js')
: './index.js',
? path.posix.join('../esm', directoryPackage, 'index.mjs')
: './index.mjs',
main: topLevelPathImportsAreCommonJSModules
? './index.js'
: path.posix.join('../node', directoryPackage, 'index.js'),
Expand Down Expand Up @@ -91,7 +91,12 @@ export async function typescriptCopy({ from, to }) {
return Promise.all(cmds);
}

export async function createPackageFile() {
/**
* Creates a package.json in the build directory.
* @param {boolean} skipExportsField Whether to skip the exports field in the package.json. Only top level ESM packages are supported.
* @returns {Promise<object>}
*/
export async function createPackageFile(skipExportsField = false) {
const packageData = await fse.readFile(path.resolve(packagePath, './package.json'), 'utf8');
const { nyc, scripts, devDependencies, workspaces, ...packageDataOther } =
JSON.parse(packageData);
Expand All @@ -104,9 +109,9 @@ export async function createPackageFile() {
main: fse.existsSync(path.resolve(buildPath, './node/index.js'))
? './node/index.js'
: './index.js',
module: fse.existsSync(path.resolve(buildPath, './esm/index.js'))
? './esm/index.js'
: './index.js',
module: fse.existsSync(path.resolve(buildPath, './esm/index.mjs'))
? './esm/index.mjs'
: './index.mjs',
}
: {}),
};
Expand All @@ -116,6 +121,38 @@ export async function createPackageFile() {
newPackageData.types = './index.d.ts';
}

if (!skipExportsField) {
newPackageData.exports = {};

const hasIndexMjs = fse.existsSync(path.resolve(buildPath, './index.mjs'));
if (hasIndexMjs) {
// Asumes the types file and node build are set up correctly
newPackageData.exports['.'] = {
types: './index.d.ts',
import: './index.mjs',
default: './node/index.js',
};
}

const hasNestedIndexFiles = glob.sync('*/index.mjs', { cwd: buildPath }).length > 0;
if (hasNestedIndexFiles) {
// Asumes the types files and node build are set up correctly
newPackageData.exports['./*'] = {
types: './*/index.d.ts',
import: './*/index.mjs',
default: './node/*/index.js',
};
}

if (!hasIndexMjs && !hasNestedIndexFiles) {
// Ensure that the packages we expect to have an exports field are set up correctly
// It avoids, for example, a top level non index file breaking the configuration
DiegoAndai marked this conversation as resolved.
Show resolved Hide resolved
throw new Error(
'Attempted to add exports field but no index.mjs files found. Adding the exports field is only supported for top level ESM packages, please check your build setup. If you want to skip adding the exports field, pass `--skipExportsField` to the script.',
);
}
}

const targetPath = path.resolve(buildPath, './package.json');

await fse.writeFile(targetPath, JSON.stringify(newPackageData, null, 2), 'utf8');
Expand Down
Loading
Loading