-
-
Notifications
You must be signed in to change notification settings - Fork 32.5k
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
Changes from 6 commits
1646c6f
365aaea
a8a9c75
330c04b
f3f6a46
bf84a60
91383ac
222eef3
bee0004
9b37c2a
d7d133d
a28200d
9fe59d8
74cb37c
6bda919
6bad76d
5d0050b
35a501e
90a3325
b94b4a6
6964f8b
7442410
fb7a4ff
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -49,5 +49,10 @@ | |
"@types/react": { | ||
"optional": true | ||
} | ||
}, | ||
"exports": { | ||
".": { | ||
"types": "./index.d.ts" | ||
} | ||
} | ||
} |
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
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, | ||
|
@@ -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', | ||
|
@@ -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) => { | ||
|
@@ -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', { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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(); |
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I will implement it. |
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.
Is there a reason to not always add the extensions?
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.
The
regressions
androllup
(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?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.
Ideally al tests use builds that are as close to the real build as possible
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.
That makes sense, I'll look into adapting the
regressions
builds as well 👍🏼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.
Hey @Janpot, I investigated the issue a bit. The
regressions
build usesbabel.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 thesrc
of the packages instead of thebuild
(see the regressions webpack config and the base webpack config).I was able to use the packages builds instead of the
src
code by doingBut even after doing that I still had to filter out some files and not add the file extensions to them:
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
MUI_ADD_IMPORT_EXTENSIONS
flag for this PR, adding the extensions only when building usingbuild.mjs
src
for the regression tests. In that issue's PR, we could revisit removingMUI_ADD_IMPORT_EXTENSIONS
and modifying the regression infrastructure accordingly.What do you think?
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.
Ok for me