Try pnpm #37324
Try pnpm #37324
Conversation
youknowriad
force-pushed
the
try/pnpm
branch
from
13ae150
to
ffb86a9
Compare
|
There's a weird typescript issue here that is causing most CI jobs to fail.
@sirreal @gziolo any idea why this happens and if there's a workaround. |
|
I'm not really sure what the issue is, I'm not familiar with pnpm or Reakit. It seems to be related to this issue microsoft/TypeScript#42873 The message says "A type annotation is necessary," providing one seems to fix the problem: diff --git a/packages/components/src/flyout/styles.ts b/packages/components/src/flyout/styles.ts
index b18cf6524d..3ac211d808 100644
--- a/packages/components/src/flyout/styles.ts
+++ b/packages/components/src/flyout/styles.ts
@@ -1,7 +1,7 @@
/**
* External dependencies
*/
-import styled from '@emotion/styled';
+import styled, { StyledComponent } from '@emotion/styled';
// eslint-disable-next-line no-restricted-imports
import { Popover as ReakitPopover } from 'reakit';
@@ -12,7 +12,9 @@ import { Card, CardBody } from '../card';
import * as ZIndex from '../utils/z-index';
import CONFIG from '../utils/config-values';
-export const FlyoutContentView = styled( ReakitPopover )`
+export const FlyoutContentView: StyledComponent<
+ React.ComponentPropsWithoutRef< typeof ReakitPopover >
+> = styled( ReakitPopover )`
z-index: ${ ZIndex.Flyout };
box-sizing: border-box;
opacity: 0;(I think that's correct, but I'm not familiar with emotion or Reakit 🙂 ) cc: @ciampo who may be able to check my work |
youknowriad
force-pushed
the
try/pnpm
branch
from
e5e3c73
to
a0484c0
Compare
Thank you for the ping Jon! I am not entirely sure if we need to exclude |
|
Let's make sure it isn't related to the fact that |
|
The types are now fixed per @sirreal's suggestion, the e2e test are passing 🎉 still have some issues to solve for unit and mobile tests (probably missing dependencies) |
|
I opened also #37396 to address some more issues discovered in this branch. |
Props to @youknowriad to fixing all those subtle bug while exploring pnpm in Gutenberg with #37324.
Yeah! Reakit exports |
👍 Agreed, it's worth seeing if the |
Props to @youknowriad to fixing all those subtle bug while exploring pnpm in Gutenberg with #37324.
Props to @youknowriad to fixing all those subtle bug while exploring pnpm in Gutenberg with #37324.
youknowriad
force-pushed
the
try/pnpm
branch
from
7ab301c
to
3c9d6c7
Compare
|
Size Change: +607 B (0%) Total Size: 1.13 MB
ℹ️ View Unchanged
|
|
I just discovered this and I just want to say that I would very much like this to happen. Could we share a before&after of the installation time and disk size comparison in the PR description? One thing that might be worth doing is to only allow pnpm, so that people won't accidentally install a package using |
|
Just wanted to share that I'm not working on this anymore, anyone can feel free to take it over. I think it's valuable to update to use pnpm (or else npm latest) |
|
I'll take a look at this today and see if I can make any extra headway. We've benefitted a lot from using |
I'm not sure how these work, so I will flag them explicitly for review. I suspect the difference just comes from how pnpm is resolving dependencies vs how npm was doing it, but just in case this actually indicates some erroneous dependency resolution we should double check it.
Can you elaborate on the benefits? How does it compare to npm 8? Do you hoist all packages to the top of the project? |
We were on npm 8 before pnpm. The main benefit is super fast installs locally when dependencies change. The first time I installed the Gutenberg dependencies with Switching to The PR where Openverse switched to pnpm is here: WordPress/openverse-frontend#525 We do hoist. Vue requires this for now (I believe Vue 3 and the latest Nuxt versions might fix this but we're stuck on Nuxt 2 and Vue 2 for now until the Nuxt upgrade path is more sensible and stable). We also had to enable A significant difference for Openverse is that our repository is not a monorepo with workspaces. The openverse-frontend repository follows the normal Nuxt structure of a monolith app with no I'll need to do some reading into how pnpm wants to manage workspaces and make sure that there's nothing specifically different that needs to be done. Meanwhile, enabling pre and post scripts fixes a lot of the errors seen in the CI on the latest commit of this branch as of writing this comment. |
sarayourfriend
force-pushed
the
try/pnpm
branch
from
352a1c5
to
6bd27fd
Compare
|
@sarayourfriend, thank you for a detailed response. I will process it early next week 👍🏻 I had a branch ready #36041 with changes for npm 8, but it doesn't seem to install correctly with GitHub Actions. It worked locally without any issues. So you can't compare install times using CI, for now. 😅 We could still be able to run some comparisons locally. |
It feels like the biggest challenge would be to update all commands from I took a look at CI jobs like https://github.com/WordPress/gutenberg/runs/4898743594?check_suite_focus=true that run |
To be fair, the CI action you're looking at there is running cachelessly as there's a brand new I think that a "raw, from-zero" install using pnpm isn't going to be faster than npm as the main bottle neck is the network, not putting the files into node_modules. For the record, this is the caching we use for Openverse: https://github.com/marketplace/actions/setup-pnpm#use-cache-to-reduce-installation-time. We just follow the official recommendation for pnpm. |
|
Taking another look at this PR today. Going to try to start from fresh and make more incremental changes. Obviously some of the changes I made broke things more than helped! I've also learned since that React Native doesn't play well necessarily with Most likely I will start a new branch and run a fresh |
There are also issues with React Native forked dependencies and npm 8 on CI. It works correctly locally. |
|
I made a lot of progress today in #38624. It's far from perfect and there are blockers (compressed-size-check doesn't support |
Related to #36142 and #37318. Alternative to #36041.