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] Bump monorepo #11303

Merged
merged 65 commits into from
Jan 10, 2024
Merged

[core] Bump monorepo #11303

merged 65 commits into from
Jan 10, 2024

Conversation

alexfauquette
Copy link
Member

@alexfauquette alexfauquette commented Dec 4, 2023

Import the modification from mui/material-ui#39808

Main modification

the file generated by docs:api don't have the same structure as before, but that's a feature, not a bug. We are now forced to be aligned with core <3

The data grid is the one with the most custom behavior. Here are the modification I made:

  • The detection of pattern @see Sett the {@link ....} for more details is now handled by the core repository
  • The chainProptypes is removed. It was causing the recast parser fail. I preferred to remove them than spending maybe half a day to debug why it's causing issues. I copy past them at the end if someone want to bring them back (agreed with @cherniavskii on that point).
  • The script that replace [[GridColDef]] by a link to the API page of grid col def interface got replaced as follow.
    • docs:api run first script from the core, then few custom script for the data grid (the one for interfaces, selectors and event).
    • In the custom script, after generating all the interfaces, I loop on translation files to replace [[...]] patterns by the correct link
    • Nitpick: I replace [[GridColDef[]]] by [[GridColDef]][] to let it work. Before/After:
      image
      image

Some TS error are not spot by the CI. I assume it's unrelated to the docs:api but due to some other update in the monorepo. So I'm fixing them (they are commented in the PR):

  • The useSLotProps needs to get the typing of additionalProps to work in the pickers, because some of those props are required in the generated object.
  • Some demo of the data grid use Rating with placeholder that does not exist in the Rating typing.

"Free" improvement

The core script do some stuff such as:

  • make sure the default defined by @default defaultValue and the const { propName=defaultValue } = props; are the same.
  • make sure that every API page is linked to a demo page (I fixed the missing one)

removed chainProptypes

Here are the chain proptypes that got removed.

 autoPageSize: chainPropTypes(PropTypes.bool, (props: any) => {
    if (props.autoHeight && props.autoPageSize) {
      return new Error(
        'MUI: The `autoPageSize` prop will conflict with `autoHeight` prop when both of them are enabled.',
      );
    }
    return null;
  }),
columns: chainPropTypes(PropTypes.array.isRequired, (props) => {
    // @ts-ignore because otherwise `build:api` doesn't work
    if (props.columns && props.columns.some((column) => column.resizable)) {
      return new Error(
        [
          `MUI: \`column.resizable = true\` is not a valid prop.`,
          'Column resizing is not available in the MIT version.',
          '',
          'You need to upgrade to DataGridPro or DataGridPremium component to unlock this feature.',
        ].join('\n'),
      );
    }
    return null;
  }),
 pagination: (props: any) => {
    if (props.pagination === false) {
      return new Error(
        [
          'MUI: `<DataGrid pagination={false} />` is not a valid prop.',
          'Infinite scrolling is not available in the MIT version.',
          '',
          'You need to upgrade to DataGridPro or DataGridPremium component to disable the pagination.',
        ].join('\n'),
      );
    }
    return null;
  },

@Janpot Janpot requested review from Janpot and removed request for Janpot December 4, 2023 10:29
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Dec 4, 2023
Copy link

github-actions bot commented Dec 4, 2023

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@Janpot
Copy link
Member

Janpot commented Dec 4, 2023

🙂 Exactly the problem I described during the retreat:

We added a npm dependency @mui/monorepo, its dependencies are installed by yarn as transitive dependencies. But the fact that it's a yarn workspace is completely irrelavant for this process. As all the subfolders of @mui/monorepo/packages are not seen as dependencies and therefore, their dependencies are not resolved. Hence, to make sure their dependencies resolve to something we faked it by installing them in the MUI X workspace. This makes them appear in the node_modules folder, but without any guarantee about version. Therefore, the remark version installed in MUI X is v15 and the one in monorepo is v13. v13 has cjs format and v15 has ESM format. Additionally, another dependency unist-util-visit is needed. But it only works with outdated v2 and not with recent v5.

This makes it run for me (up until a runtime failure, but I'm leaving that for you):

diff --git a/package.json b/package.json
index 51a9e13aa..439cfc3f0 100644
--- a/package.json
+++ b/package.json
@@ -159,12 +159,13 @@
     "process": "^0.11.10",
     "react": "^18.2.0",
     "react-dom": "^18.2.0",
-    "remark": "^15.0.1",
+    "remark": "^13.0.0",
     "serve": "^14.2.1",
     "sinon": "^16.1.3",
     "stream-browserify": "^3.0.0",
     "string-replace-loader": "^3.1.0",
     "typescript": "^5.2.2",
+    "unist-util-visit": "^2.0.3",
     "util": "^0.12.5",
     "webpack": "^5.89.0",
     "webpack-cli": "^5.1.4",

This is still the most brittle setup. It will break in the blink of an eye and only works by accident. As long as we don't get rid of the @mui/monorepo github dependency and start publishing packages we will keep wasting countless hours on debugging these problems and finding workarounds. I know I have wasted many already 🙂

cc @michaldudak This problem will likely compound the more internal packages we create. We may want to think about setting up publishing before creating more?

@alexfauquette
Copy link
Member Author

I managed to move a bit on the topic.

A few Errors were interesting since they allowed me to add some information. But some would require a lot of work from X team to have an architecture similar to the core team.

slots are not detected because our interfaces end with xxxSlotsComponent (@cherniavskii @flaviendelangle what about renaming them xxxSlots now that components prop get removed)

Stil to do

  • Makes it works for charts and data grid
  • double check the file structure. For translation we have a different level of nested file which complicate the git diff

@flaviendelangle
Copy link
Member

slots are not detected because our interfaces end with xxxSlotsComponent (@cherniavskii @flaviendelangle what about renaming them xxxSlots now that components prop get removed)

If the core is doing the same then yes
If not, we should check why there aren't doing it.

@cherniavskii
Copy link
Member

@Janpot

As long as we don't get rid of the @mui/monorepo github dependency and start publishing packages

Could extracting the @mui/monorepo to a separate repo be an alternative solution? It will be easier to test changes and upgrade monorepo as we skip the publish step.

@Janpot
Copy link
Member

Janpot commented Dec 7, 2023

Could extracting the @mui/monorepo to a separate repo be an alternative solution?

I'm not sure I follow, the @mui/monorepo dependency already is a GitHub repository. Could you explain a bit more in detail the setup you have in mind?

The root of the problem is that none of the yarn workspaces in the MUI core repo are recognized as npm packages by the X yarn workspace, hence none of their dependencies are installed, and none of them can be used as a yarn dependency without setting up aliases in the build tools. Actually they can't even be used at all without a build tool. And even after setting up these aliases, the imported code is completely different from the one that is referenced on our end-users' setup.

@alexfauquette
Copy link
Member Author

If the core is doing the same then yes
If not, we should check why they aren't doing it.

Slots are from @mui/base and exported with the naming [ComponentName]Slots
For slotProps, they are defined using SlotComponentProps or SlotComponentPropsWithSlotState utilities.

https://github.com/mui/material-ui/blob/4e562c5f118272399262fcc6f5b905ae3447832e/packages/mui-base/src/Slider/Slider.types.ts/#L68-L96

@cherniavskii
Copy link
Member

@Janpot

I'm not sure I follow, the @mui/monorepo dependency already is a GitHub repository. Could you explain a bit more in detail the setup you have in mind?

Sorry, I meant extracting everything we import from @mui/monorepo to a separate repo.
This is mostly @mui/monorepo/docs and internal packages from @mui/monorepo/packages/ (like @mui/monorepo/packages/api-docs-builder).
This extracted repo won't be a monorepo anymore and everything will be treated as a single package (e.g. @mui/docs-infra).
The material-ui repo will use it as a dependency (similar to how MUI X currently uses @mui/monorepo).

Actually they can't even be used at all without a build tool.

This includes Babel transpilation and TS compilation, right? Is there something else?

@flaviendelangle
Copy link
Member

Slots are from @mui/base and exported with the naming [ComponentName]Slots
For slotProps, they are defined using SlotComponentProps or SlotComponentPropsWithSlotState utilities.

In that case yes I'm 100% in favor of unifying AND simplifying our naming

@Janpot
Copy link
Member

Janpot commented Dec 7, 2023

Sorry, I meant extracting everything we import from @mui/monorepo to a separate repo.

In npm, pnpm and yarn<2, you can only specify the repository root as a dependency. Whether that root is a yarn workspace doesn't make a difference. All the package manager sees is a npm package at the root of the github repository, it completely ignores the workspaces key in package.json. What you propose wouldn't be any different from what we have now.

(Unless you propose to create a github repository for every individual internal package)

This includes Babel transpilation and TS compilation, right? Is there something else?

Some tools don't run through a build pipeline at all, like the upcoming zero runtime next.js plugin. Or some of the markdown tooling in the docs. Some of our scripts also run natively on node.js.

But building is only half of the problem. The other problem is that when you yarn install in the X repo, the @mui/monorepo/docs/package.json is completely ignored, so any code under @mui/monorepo/docs/** either can't load the dependency if we don't install it in X root as well, or might load the wrong version depending on what's going on in the X repository dependency structure.

@cherniavskii
Copy link
Member

All the package manager sees is a npm package at the root of the github repository, it completely ignores the workspaces key in package.json.

My thought was to have no packages folder in that repo, and just a single package.json that will be resolved by the package manager.

But if we need a build step anyway, then publishing to NPM makes more sense 👍🏻
For testing, we can use CodeSandbox CI - like we do with our public packages.

@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Dec 11, 2023
@Janpot
Copy link
Member

Janpot commented Dec 11, 2023

My thought was to have no packages folder in that repo, and just a single package.json that will be resolved by the package manager.

But that would mean that one single package folder will be responsible for all of docs infra, all test infra, all linting, all build pipelines without any possibility of internally structuring this in a workspace. I think it wouldn't scale well.

@LukasTy LukasTy mentioned this pull request Dec 12, 2023
1 task
@cherniavskii cherniavskii added the core Infrastructure work going on behind the scenes label Dec 13, 2023
@cherniavskii
Copy link
Member

@alexfauquette Can I help you with something to move this PR forward?

@alexfauquette
Copy link
Member Author

@cherniavskii With pleasure.
As I said in the description, the issue comes from mui/material-ui#39808 This PR is splitting script in multiple helper, and add settings to glue them together. This lead to removing some fuction we imported in the docs:api script.

I think there are two options. A short and a long one.

  • The short is to recreate the function that were removed by the PR to make the docs:api script work again
  • The long is to use the settings to recreate the docs:api script in a way that is coherent with the core

This PR is the long one. In parallel I'm adding settings I miss to make it work in this PR.

For now, I'm near completion for date-pickers. Some slots are still missing, and the translations are destroyed after their creation I don't know why.

If you need the core bump for any reason, giving a try to the quick fix solution could worth it

@michaldudak
Copy link
Member

This problem will likely compound the more internal packages we create. We may want to think about setting up publishing before creating more?

Sure thing. We can discuss the priorities on the code infra meeting, but I'm on board with having npm publishing done soon.

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Dec 15, 2023
Copy link

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Jan 9, 2024
package.json Outdated Show resolved Hide resolved
@alexfauquette
Copy link
Member Author

@cherniavskii Not sure we can trick the codecov. I've no idea why those code coverage decreased. But coverage modification i huge compare to the number of file modified

Comparing next branch / this PR
image

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Jan 9, 2024
Copy link

github-actions bot commented Jan 9, 2024

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Jan 10, 2024
trying to find out why codecov started ignoring test files that caused coverage drop
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.

7 participants