material-ui icon indicating copy to clipboard operation
material-ui copied to clipboard

[core] Add exports field to packages

Open DiegoAndai opened this issue 11 months ago • 11 comments

Add exports field add .mjs extension to ESM files on the packages that use the common pipeline build.mjs/copyFiles.mjs except for:

  • @mui/icons-material: Will be handled in https://github.com/mui/material-ui/issues/35233
  • @mui/codemod: Custom structure
  • @mui/docs: No exports
  • @mui/downloads-tracker: No exports

Besides that, fix/adapt other configurations and imports to this new structure.

DiegoAndai avatar Mar 21 '24 19:03 DiegoAndai

Netlify deploy preview

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

@mui/joy/MenuList: parsed: +0.90% , gzip: +1.02% @mui/joy/Chip: parsed: +0.98% , gzip: +1.16% RadioGroup: parsed: +1.32% , gzip: +1.45% @mui/joy/ListSubheader: parsed: +1.17% , gzip: +1.13% @mui/joy/RadioGroup: parsed: +1.16% , gzip: +1.14% @mui/joy/Menu: parsed: +0.59% , gzip: +0.80% @mui/joy/Drawer: parsed: +0.81% , gzip: +0.96% Dialog: parsed: +0.89% , gzip: +0.98% @mui/joy/Tooltip: parsed: +0.73% , gzip: +0.84% @mui/joy/MenuItem: parsed: +1.00% , gzip: +0.99% @mui/joy/Accordion: parsed: +1.00% , gzip: +1.22% Rating: parsed: +1.12% , gzip: +1.15% @mui/joy/Option: parsed: +0.98% , gzip: +1.03% @mui/joy/FormControl: parsed: +1.07% , gzip: +1.22% @mui/joy/ModalDialog: parsed: +0.91% , gzip: +0.90% Tooltip: parsed: +0.76% , gzip: +0.70% @mui/joy/Tab: parsed: +0.97% , gzip: +0.95% @mui/joy/TabPanel: parsed: +1.08% , gzip: +1.17% Hidden: parsed: +1.29% , gzip: +1.24% @mui/joy/Checkbox: parsed: +0.91% , gzip: +0.94% and 23 more changes

Bundle size report

Details of bundle changes (Toolpad) Details of bundle changes

Generated by :no_entry_sign: dangerJS against fb7a4ff225df4418b9beb263a3aa6c78610ca1c0

mui-bot avatar Mar 21 '24 19:03 mui-bot

Hey @Janpot! A couple of things:


I added instructions for possible breaking changes of custom configurations:

  • https://deploy-preview-41596--material-ui.netlify.app/material-ui/migration/migration-v5/
  • https://deploy-preview-41596--material-ui.netlify.app/system/migration/migration-v5/

Is there anything else we should add to that?


I'm wondering, do we need to add the modern build to the exports field?:

"exports": {
    "./modern": {
        "types": "./modern/index.d.ts",
        "default": "./modern/index.mjs",
    },
    "./modern/*": {
        "types": "./modern/*/index.d.ts",
        "default": "./modern/*/index.mjs",
    },
    ".": {
        "types": "./index.d.ts",
        "import": "./index.mjs",
        "default": "./node/index.js"
    },
    "./*": {
        "types": "./*/index.d.ts",
        "import": "./*/index.mjs",
        "default": "./node/*/index.js"
    }
}

To support https://next.mui.com/material-ui/guides/minimizing-bundle-size/#modern-bundle?

We won't need it for the legacy build as that's to be removed: #40958

DiegoAndai avatar Mar 27 '24 13:03 DiegoAndai

Is there anything else we should add to that?

I don't think so, this seems clear to me.

I'm wondering, do we need to add the modern build to the exports field?:

If we want to keep supporting the modern bundle it we will likely need to do that. We can add a modern field? Not sure whether there already is a consensus around a field. In any case this will be a breaking change for users that were aliasing in their bundler to get to the modern version. We will need to update how they'd have to configure their bundlers. Modern bundlers have settings that allow you to give priority to a custom exports field:

  • webpack: https://webpack.js.org/configuration/resolve/#resolveconditionnames
  • vite: https://v2.vitejs.dev/config/#resolve-conditions
  • esbuild: https://esbuild.github.io/api/#conditions

This will also need to go in the migration guide then

If instead we want to keep supporting the modern bundle through aliasing we will have to add exports for them in the package.json

"exports": {
  "./modern": {
    ...
  },
  "./modern/Button": {
    ...
  }
} 

But I'd avoid that as per https://next.mui.com/material-ui/guides/minimizing-bundle-size/#how-to-use-custom-bundles

Janpot avatar Mar 27 '24 14:03 Janpot

So, if I understand correctly, the options are:

1. Add a modern condition inside exports:

"exports": {
    ".": {
        "types": "./index.d.ts",
        "modern": "./modern/index.mjs"
        "import": "./index.mjs",
        "default": "./node/index.js"
    },
    "./*": {
        "types": "./*/index.d.ts",
        "modern": "./modern/*/index.mjs",
        "import": "./*/index.mjs",
        "default": "./node/*/index.js"
    }
}
  • Pros:
    • Doesn't expose /modern. This would allow us to remove the warning in https://next.mui.com/material-ui/guides/minimizing-bundle-size/#how-to-use-custom-bundles.
    • Supported in bundlers (https://github.com/mui/material-ui/pull/41596#issuecomment-2022851323)
  • Cons:
    • It would be a breaking change for users using the modern build. They would have to reconfigure their bundlers. We could probably provide a codemod. (cc: @mnajdova I'm curious how bad of a breaking change you think this would be, I don't think it's too bad)

2. Add ./modern and ./modern/* patterns inside exports:

"exports": {
    "./modern": {
        "types": "./modern/index.d.ts",
        "default": "./modern/index.mjs",
    },
    "./modern/*": {
        "types": "./modern/*/index.d.ts",
        "default": "./modern/*/index.mjs",
    },
    ".": {
        "types": "./index.d.ts",
        "import": "./index.mjs",
        "default": "./node/index.js"
    },
    "./*": {
        "types": "./*/index.d.ts",
        "import": "./*/index.mjs",
        "default": "./node/*/index.js"
    }
}
  • Pros:
    • No breaking changes
  • Cons:
    • Exposes ./modern

I would opt for 1., I don't think the breaking change is too bad. If we had the exports field back when the modern build was implemented, I would guess 1. is how it would've been implemented to avoid exposing /modern.

DiegoAndai avatar Mar 27 '24 14:03 DiegoAndai

@Janpot, this is ready for review once more 😊

  • For the compatibility mode, I refactored to use the process.env.MUI_SKIP_CORE_EXPORTS_FORMAT flag, which allows to disable all changes with a single flag (example) as well as more granular control (example). This should allow the X team to adopt the changes gradually.
  • For the modern build, I went with option 1. I added a section to the migration guide and updated the instructions as well. It's working as expected on Vite. I will also test on the other bundlers before merging. One issue with this approach is that it's a project-wide config and not per-dependency, so if another package uses the modern condition name, there might be a conflict, but I don't think it's too probable. Let me know if we should use option 2 instead.

DiegoAndai avatar Apr 15 '24 16:04 DiegoAndai

To be clear, the compatibility mode is an opt-out, not an opt-in, right? Would it be hard to make this an opt-in instead? That way we loosely couple the migration of X to this system from the merge of this PR.

Otherwise this PR seems good.

On the changes of the modern bundle I'd like to put this to attention of @michaldudak and @mnajdova to be aware of this and a final blessing.

Janpot avatar Apr 16 '24 10:04 Janpot

One concern that I have is that the "modern" condition may be used by another package as well, and since we can't control conditions per import, configuring a bundler to use "modern" will change imports in both MUI and 3rd party packages. Having the condition more specific (like "mui-modern") could reduce the risk of this problem.

michaldudak avatar Apr 16 '24 12:04 michaldudak

To be clear, the compatibility mode is an opt-out, not an opt-in, right? Would it be hard to make this an opt-in instead? That way we loosely couple the migration of X to this system from the merge of this PR.

Exactly. We can make it opt-in 👍🏼 I think it will be safer. I'll implement it.

Having the condition more specific (like "mui-modern") could reduce the risk of this problem.

I'll implement this as well.

I'll let you know when this changes are ready for review.

DiegoAndai avatar Apr 16 '24 12:04 DiegoAndai

@Janpot ready for review 😊

DiegoAndai avatar Apr 16 '24 18:04 DiegoAndai

@samuelsycamore I added you for copy review of the migration guide and updated bundle instructions.

DiegoAndai avatar Apr 16 '24 19:04 DiegoAndai

I'm doing some final testing. Please don't merge yet.

DiegoAndai avatar Apr 17 '24 20:04 DiegoAndai

Closing this PR as per: https://github.com/mui/material-ui/issues/30671#issuecomment-2130387754.

DiegoAndai avatar May 24 '24 21:05 DiegoAndai