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

[code-infra] Change package manager to pnpm #11875

Merged
merged 104 commits into from
Apr 24, 2024
Merged

Conversation

LukasTy
Copy link
Member

@LukasTy LukasTy commented Jan 31, 2024

Closes #10658

CI runtime improvement

Before: ~20mins
After: ~16mins

Replicate the migrations already done on mui/material-ui#36287 and mui/toolpad#2546.

IMHO, this change https://github.com/mui/material-ui/pull/36287/files#diff-8ee0a5d567f55f7e7507f6cd79d9c37721906efe84f500acc89c551508c43464 deserves to be delivered via a separate PR to reduce the unwanted noise. 🤔

Progress

  • Switch yarn to pnpm
  • Make eslint happy
  • Make proptypes happy
  • Make typescript happy
  • Make build happy
  • Make test:unit happy
  • Make test:karma happy
  • Make test:e2e happy
  • Make test:regressions happy
  • Make docs:api happy
  • Test all them scripts
    Tested more or less all scripts, only size:snapshot and size:why do not work. But that's the case with yarn as well. 🙈

Build output comparison

There are 732 files with diffs (some of them expected - package.json) comparing the current build output with yarn and this branch.

The most notable changes repeated in many files in most packages:

  1. {} instead of Object.prototype:
-function _interopRequireWildcard(e, r) { if (!r && e && e.__esModule) return e; if (null === e || "object" != typeof e && "function" != typeof e) return { default: e }; var t = _getRequireWildcardCache(r); if (t && t.has(e)) return t.get(e); var n = { __proto__: null }, a = Object.defineProperty && Object.getOwnPropertyDescriptor; for (var u in e) if ("default" !== u && Object.prototype.hasOwnProperty.call(e, u)) { var i = a ? Object.getOwnPropertyDescriptor(e, u) : null; i && (i.get || i.set) ? Object.defineProperty(n, u, i) : n[u] = e[u]; } return n.default = e, t && t.set(e, n), n; }
+function _interopRequireWildcard(e, r) { if (!r && e && e.__esModule) return e; if (null === e || "object" != typeof e && "function" != typeof e) return { default: e }; var t = _getRequireWildcardCache(r); if (t && t.has(e)) return t.get(e); var n = { __proto__: null }, a = Object.defineProperty && Object.getOwnPropertyDescriptor; for (var u in e) if ("default" !== u && {}.hasOwnProperty.call(e, u)) { var i = a ? Object.getOwnPropertyDescriptor(e, u) : null; i && (i.get || i.set) ? Object.defineProperty(n, u, i) : n[u] = e[u]; } return n.default = e, t && t.set(e, n), n; }
  1. Changed attributes order in d.ts files and removal of reference comments (i.e. /// <reference types="@react-spring/shared" />)
  2. Combine jsx-runtime imports.

Note

This looks like a "bugfix" to me.

-import { jsx as _jsx } from "react/jsx-runtime";
-import { jsxs as _jsxs } from "react/jsx-runtime";
+import { jsx as _jsx, jsxs as _jsxs } from "react/jsx-runtime";
  1. Remove explicit undefined for optional fields, i.e.:

Note

This is due to a bump of typescript. Bumping it on yarn produces the same result.

packages/x-data-grid/build/hooks/features/columnHeaders/useGridColumnHeaders.d.ts
-pinnedPosition?: GridPinnedColumnPosition | undefined;
+pinnedPosition?: GridPinnedColumnPosition;
  1. Use eligible imports in GridPremiumColumnMenu.d.ts:

Note

This is due to a bump of typescript. Bumping it on yarn produces the same result.

Screenshot 2024-04-23 at 18 21 13 This looks like a bugfix to me. 🤔 P.S. `This branch` build on the **right** and `master` build on the **right**.

The exploration for ditching babel-loader (no longer relevant as the problem has been fixed in a different way):

  • The easiest way to make tests happy is by changing babel to tsx for test compilation
    • This is not trivial because we want to keep testing both date-fns@v2 and date-fns@v3 adapters on Pickers side and their testing implementation currently relies on hackily replacing imports on our predefined rules using a patched babel plugin.
    • I've explored creating a separate package just to test date-fns@v3, but that doesn't work nicely, because the resolved version of date-fns is still v2, because it comes (is used) from a dev dependency on @mui/x-date-pickers event though this package specifies and resolves "date-fns": "^3.6.0". 🙈

@LukasTy LukasTy added core Infrastructure work going on behind the scenes scope: code-infra Specific to the core-infra product labels Jan 31, 2024
@LukasTy LukasTy self-assigned this Jan 31, 2024
"tsx": "^4.7.0",
"typescript": "^5.3.3",
"typescript": "^5.4.5",
Copy link
Member Author

Choose a reason for hiding this comment

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

Had to bump to avoid problems with TS being unable to correctly resolve linked deps in a monorepo.
microsoft/TypeScript#42873 (comment)

@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Apr 22, 2024
@@ -8,9 +8,16 @@
"noEmit": false,
"emitDeclarationOnly": true,
"outDir": "build",
"rootDir": "./src"
"rootDir": "./src",
"types": [
Copy link
Member

@Janpot Janpot Apr 22, 2024

Choose a reason for hiding this comment

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

Yes, this is great! Is there a reason to not place it directly in tsconfig.json? I feel like it would be beneficial to keep the config for typechecking as close as possible to the one for type generation. This file should adopt the setting automatically through the extends.

Copy link
Member

Choose a reason for hiding this comment

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

I never really understood why we have a separate tsconfig for the build.
@LukasTy @Janpot Can you enlighten me, please? 🙂

Copy link
Member

@Janpot Janpot Apr 22, 2024

Choose a reason for hiding this comment

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

If I understand correctly, two reasons

  1. for typechecking we use noEmit: true and for type generation we use noEmit: false. But I believe this should be achievable through a cli flag as well.
  2. for type generation we're working towards a setup with a single compilation using project references instead of a compilation per project. But I hope we can bring this setup to type checking as well.

cc @michaldudak will be able to shed some more light, I may be a bit off on this.

Copy link
Member

Choose a reason for hiding this comment

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

But I hope we can bring this setup to type checking as well.

This is already set up like this in the Base UI repo. However, it doesn't work perfectly (yet), as it doesn't typecheck tests.

for typechecking we use noEmit: true and for type generation we use noEmit: false. But I believe this should be achievable through a cli flag as well.

True. I believe all settings are available through commandline flag. It's a matter of readability to create a config file.

In addition to what @Janpot wrote, the build config also excludes tests from the output.

Copy link
Member

Choose a reason for hiding this comment

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

Any ideas why I can't install dependencies?
I've removed my node_modules before pnpm install

Copy link
Member

Choose a reason for hiding this comment

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

Did you use corepack enable to install pnpm? Can you try that?

Otherwise, if you installed it separately, you may take a look at this issue and remove pnpm from your PATH variable and let corepack take over.

Copy link
Member

Choose a reason for hiding this comment

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

I installed using this script https://pnpm.io/8.x/installation#on-posix-systems

you may take a look at pnpm/pnpm#3016 (comment) and remove pnpm from your PATH variable and let corepack take over.

That didn't help, but sudo corepack enable pnpm did, thanks!

Copy link
Member

@cherniavskii cherniavskii left a comment

Choose a reason for hiding this comment

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

Looks good as far as I'm concerned!
Thanks for working on this! 🎉

Copy link
Member

@Janpot Janpot left a comment

Choose a reason for hiding this comment

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

Great effort!

@@ -5,6 +5,9 @@
> Tip: For people who are doing the release for the first time, make sure you sign in to npm from the command line using security-key flow as well as have two-factor authentication enabled.
> You can follow [this guide](https://docs.npmjs.com/accessing-npm-using-2fa) for more information on how to set it up.

> Tip: You can use `release:publish:dry-run` to test the release process without actually publishing the packages.
Copy link
Member Author

Choose a reason for hiding this comment

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

@michaldudak I've added a tip here to spread more knowledge as I wasn't aware of this option before working on this PR. 🙈

@LukasTy LukasTy merged commit 931fd6e into mui:master Apr 24, 2024
17 checks passed
@LukasTy LukasTy deleted the yarn-to-pnpm branch May 17, 2024 15:02
DungTiger pushed a commit to DungTiger/mui-x that referenced this pull request Jul 23, 2024
Signed-off-by: Lukas <llukas.tyla@gmail.com>
Co-authored-by: Jose Quintas <juniorquintas@gmail.com>
thomasmoon pushed a commit to thomasmoon/mui-x that referenced this pull request Sep 9, 2024
Signed-off-by: Lukas <llukas.tyla@gmail.com>
Co-authored-by: Jose Quintas <juniorquintas@gmail.com>
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 scope: code-infra Specific to the core-infra product
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[core] Migrate from yarn to pnpm
7 participants