-
Notifications
You must be signed in to change notification settings - Fork 31
[code-infra] Support flat build output behind a flag #1064
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
base: master
Are you sure you want to change the base?
Conversation
✅ Deploy Preview for mui-internal ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Bundle size report
Check out the code infra dashboard for more information about this PR. |
859d9e3 to
46312cb
Compare
| The executable files that should be installed into the `PATH`. | ||
| */ | ||
| bin?: string | Partial<Record<string, string>>; | ||
| bin?: string | Record<string, string>; |
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.
This would cause pkg.bin['any string here'] to be of type string. We want it to be string | undefined.
| bin?: string | Record<string, string>; | |
| bin?: string | Partial<Record<string, string>>; |
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.
But isn't that the case ? If pkg.bin has a key, it should have a value. It can't be undefined. Or maybe I am missing some use-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.
The way this is declared has a reason. It's to maximize type safety. Consider the following example under your changes:
function checkBin(pkg: PackageJson) {
const foo: string = pkg.bin['some nonsense']
console.log(typeof foo);
// prints undefined, even though we declared it as string
}Compiles without a problem and the type of foo is actually not corresponding to the actual value at runtime. this is a common source of bugs. If we do Partial, then it will complain about assigning something that can potentially be undefined to a string.
Moreover, it requires you to be defensive, typescript forces you to add a check whether the property exists before blindly using it:
function checkBin(pkg: PackageJson) {
if (pkg.bin['some-script']) {
const path: string = pkg.bin['some-script']
}
}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.
Do you mean that there might be a case that someone has not specified a value for a key in package.json['bin'] ? Like this -
"bin": {
"code-infra": null // since there cannot be undefined
}I don't see this happening but I have reverted any ways since it required 1-2 if checks.
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.
no, the oppositie, that someone tries to access a key that is not in that object.
|
Nice work. I think you may want to check mui-public/packages/code-infra/src/utils/babel.mjs Lines 148 to 151 in adc9a37
It's to handle an edge case with Next.js used here: https://github.com/mui/material-ui/blob/b09d704dcdc0de3ea0d54b10e1c0803e8256b415/packages/mui-material-nextjs/src/v13-appRouter/appRouterV13.tsx#L6 I suspect that:
Not sure though |
56c0ac3 to
414402a
Compare
9687adf to
da754b7
Compare
d8d3202 to
744aff6
Compare
|
Any news on this? It's a critical fix to make MUI work with Module Federation currently |
|
Although this has been implemented, we need to test the build output with all the bundlers to make sure that it won't introduce any bugs. |
75e8dac to
0ef8561
Compare
|
@Janpot The PR is now ready. I've posted a detailed bundler comparison in Base UI PR. |
|
Does this work with codesandbox now? Is it portable to Core/X? |
|
No. No codesandbox still. I was just trying to make sure anything that already works is not breaking. Codesandbox doesn't work with current structure as well as the new one.
Yes. Same structure for all our packages. I analyzed just Base UI since it was already setup with Michal's bundler project. |
| await createFile( | ||
| path.join( | ||
| outputDir, | ||
| `index${getOutExtension('esm', { isFlat: true, packageType: 'module' })}`, |
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 would avoid using getOutExtension in fixtures. If there's a bug in getOutExtension, it may cancel each other out in the implementation details. Instead, make the fixture more static and explicit:
`index.mjs`,Tests must be as simple as possible, we don't want to have to create tests for tests.
Can these files be created in parallel?
Are these tests written by AI?
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. It has followed what was in the AGENTS file, ie, to make the tests closer to the real world. That's why its actually creating the files instead of simulating a filesystem.
1. New cli arg `--flat` for the new logic to create flat builds 2. Flat builds create `mjs` (and `d.mts` for types) files for esm and `cjs` (and `d.cts` for types) files for cjs builds for simplicity. There are no `js` files now. 3. Added validation for `bin` field in `package.json`. It should point to the source files instead of built files now.
0ef8561 to
06c1655
Compare
06c1655 to
4286afa
Compare
|
@csvan Could you verify if module federation related issues are resolved if you use |
this will remove matching exports from the final package.json
e7d71e4 to
e35e0f9
Compare
The package might be trying to block an import
New cli arg
--flatfor the new logic to create flat buildsFlat builds create. I scrapped this due to an issue withmjs(andd.mtsfor types) files for esm andcjs(andd.ctsfor types) files for cjs builds for simplicity. There are nojsfiles now.tsccompilation where it needs the presence ofd.tsfiles which was not present with this (only.d.ctsand.d.mtsfiles were present). This can perhaps be achieved when we move over to use a bundler but not currently.Flat builds use the type value of
package.jsonto determine the output file extensions.For
module-.jsfiles are es modules and.cjsfiles are commonjs.mainfield points toindex.cjs(if export['.'] is present andmodulefield points toindex.jsFor
commonjsor when no type is specified -.jsfiles are commonjs and.mjsfiles are es modules.mainfield points toindex.js(if export['.'] is present andmodulefield points toindex.mjsAdded validation for
binfield inpackage.json. It should point to the source files instead of built files now.Some refactoring/tests around
exportsandbingeneration for outputpackage.json.To fix:
tscbuild with project references throws errorIn an effort to fix issues like -
Downstream changes -