andresz1 / size-limit-action

Compare the real cost to run your JS app or lib to keep good performance in every pull request
ISC License
449 stars 83 forks source link

add option to specify package manager #77

Open phryneas opened 2 years ago

phryneas commented 2 years ago

Recent builds of https://github.com/reduxjs/redux-toolkit/ seem to fail with size-limit-action, since size-limit-action does not detect the yarn monorepo (yarn.lock is in the root folder, we are running with a different directory though) and runs npm instead.

Would it be possible to add a flag to just skip all the flaky autodetection and just manually specify the package manager? I'd be happy to file a PR.

zchenwei commented 2 years ago

+1.. It doesn't detect yarn monorepo. If you want to run size-limit with a different directory other than the root, it uses npm instead and that could be problematic.

/opt/hostedtoolcache/node/16.14.2/x64/bin/npm install
npm ERR! code ERESOLVE
npm ERR! ERESOLVE could not resolve
npm ERR! 
npm ERR! While resolving: ts-jest@[27](https://github.com/zchenwei/amplify-ui/runs/6278756848?check_suite_focus=true#step:9:27).1.4
npm ERR! Found: @types/jest@26.0.24
npm ERR! node_modules/@types/jest
npm ERR!   @types/jest@"^26.0.20" from @vue/cli-plugin-unit-jest@5.0.0-beta.2
npm ERR!   node_modules/@vue/cli-plugin-unit-jest
soanvig commented 2 years ago

So I created a temporary fork: https://github.com/soanvig/size-limit-action-yarn-v2 That just uses yarn directly. You can install it in the meantime in your GH like so: https://github.com/soanvig/mm-jsr/blob/master/.github/workflows/size-limit.yml

zchenwei commented 2 years ago

So I created a temporary fork: https://github.com/soanvig/size-limit-action-yarn-v2 That just uses yarn directly. You can install it in the meantime in your GH like so: https://github.com/soanvig/mm-jsr/blob/master/.github/workflows/size-limit.yml

@soanvig This is great! But can you add an option to customize the manager? For a monorepo, there will be only one yarn.lock in the root and has-yarn will not work if I am using it in a subdirectory

soanvig commented 2 years ago

@zchenwei I advise you to create your own fork in this case

markerikson commented 2 years ago

It seems like the recent "custom script" option in #79 could help fix this, by letting us run yarn size-limit ourselves (given that we have it as a devDep in RTK)... but it hasn't been released yet.

@andresz1 could you put out a new version with that fix?

andresz1 commented 2 years ago

Hi @markerikson! I just updated the v1 release with the changes. Let me know if it works for you please 🙏🏻 . I also added a section for this in the README

markerikson commented 2 years ago

@andresz1 thank you! I'll have to try it out here shortly.

as a fellow maintainer I hate to be the one doing the "PLZ RELEASE THISS!!!!!!!" :) appreciate the response!

andresz1 commented 2 years ago

@markerikson hahaha 🤣 no worries! BTW thank you both for your work with Redux.

andresz1 commented 2 years ago

@zchenwei @soanvig you can now use yarn dlx instead. Please let me know how it goes! (see section)

markerikson commented 2 years ago

Hmm. I just tried updating RTK's workflow to add the script option, and I'm still getting npm errors even after I explicitly specify size-limit-action@1.7.0.

Lemme actually paste in the entire error for reference:

Run andresz1/size-limit-action@v1.7.0
  with:
    directory: packages/toolkit
    github_token: ***
    build_script: build-only
    script: yarn size-limit --json
    windows_verbatim_arguments: true
  env:
    CI_JOB_NUMBER: 1
/usr/local/bin/npm install
npm ERR! code ERESOLVE
npm ERR! ERESOLVE unable to resolve dependency tree
npm ERR! 
npm ERR! While resolving: rtk-monorepo@undefined
npm ERR! Found: eslint@7.32.0
npm ERR! node_modules/eslint
npm ERR!   dev eslint@"^7.25.0" from the root project
npm ERR! 
npm ERR! Could not resolve dependency:
npm ERR! peer eslint@"^5.0.0 || ^6.0.0" from @typescript-eslint/parser@2.34.0
npm ERR! node_modules/@typescript-eslint/parser
npm ERR!   peer @typescript-eslint/parser@"2.x" from eslint-config-react-app@5.2.1
npm ERR!   node_modules/eslint-config-react-app
npm ERR!     dev eslint-config-react-app@"^5.0.1" from the root project
npm ERR!   peer @typescript-eslint/parser@"^2.0.0" from @typescript-eslint/eslint-plugin@2.34.0
npm ERR!   node_modules/@typescript-eslint/eslint-plugin
npm ERR!     peer @typescript-eslint/eslint-plugin@"2.x" from eslint-config-react-app@5.2.1
npm ERR!     node_modules/eslint-config-react-app
npm ERR!       dev eslint-config-react-app@"^5.0.1" from the root project
npm ERR! 
npm ERR! Fix the upstream dependency conflict, or retry
npm ERR! this command with --force, or --legacy-peer-deps
npm ERR! to accept an incorrect (and potentially broken) dependency resolution.
npm ERR! 
npm ERR! See /home/runner/.npm/eresolve-report.txt for a full report.

Unfortunately that output has me really confused. I assumed this was erroring while trying to do npx size-limit, but now I have no idea where that npm install is actually coming from.

Obviously the symptom here is that npm just doesn't like conflicting peer deps, but the bigger question is why npm install is happening at all.

Any ideas why? I'm not familiar with the internals of Github Actions, so I don't know if this is happening as the action itself is being installed, or something else.

andresz1 commented 2 years ago

I think that I found the issue. The problem is that auto-detecting the package manager to install dependencies is not working properly with monorepos and directory option. It tries to find a yarn.lock inside toolkit directory but there is none so it fallbacks to npm install. I will try to provide a solution ASAP (hopefully tomorrow). Let me know if you can wait and try it out again @markerikson

markerikson commented 2 years ago

Yeah, that's generally the conclusion the earlier comments were coming to :)

FWIW I just spent the last half hour trying to bump all of RTK's eslint-related devDeps to work around this, but yeah, I think this really is the root cause - npm shouldn't get run at all.

I've got the action disabled in our repo atm, but was trying to re-enable it with v1.7.0 over in https://github.com/reduxjs/redux-toolkit/pull/2374 .

If you could ping me when you think you've got this fixed, I'd appreciate it!

andresz1 commented 2 years ago

Sure! will do. Thank you for pointing out the issue and sorry for that :(

zchenwei commented 2 years ago

I think that I found the issue. The problem is that auto-detecting the package manager to install dependencies is not working properly with monorepos and directory option. It tries to find a yarn.lock inside toolkit directory but there is none so it fallbacks to npm install. I will try to provide a solution ASAP (hopefully tomorrow). Let me know if you can wait and try it out again @markerikson

Yes, I think that's the issue. I have a monorepo and want to run size limit in one of workspaces by specifying directory option but that will cause it to look for yarn.lock there as well.. Plz let me know once you've got it fixed. Thanks! :)

PilotConway commented 2 years ago

We just ran into this same issue with npm being used over yarn. I got it working with the script option using yarn workspace, but the issue we had was doing that still ran install and build across the whole monorepo which takes quite a bit of time (and compute minutes which our IT team isn't too fond of). So we really want to use the directory option to specify just the package we need to build but that introduces this issue where its trying to use npm which doesn't work at all in our repo.

I got a fork working with an option to manually override the package manager and its working in our private repo so just submitted a PR in case you are interested in incorporating that change.

inomdzhon commented 1 year ago

Hi there,

I solved problem by added fake yarn.lock file to package directory

.github/workflows/pull_request.yml

  - uses: andresz1/size-limit-action@v1.7.0
    with:
      github_token: ${{ secrets.GITHUB_TOKEN }}
      # only affects current branch
      skip_step: install
      directory: packages/my_package/
      # v1.7.0 doesn't support this property yet. See packages/my_package/yarn.lock
      # package_manager: yarn
      build_script: 'size:ci'

packages/my_package/yarn.lock

# ⚠️ FAKE yarn.lock
# 
# <I have some description here about why>
#
# TODO Remove it, after `andresz1/size-limit-action` will support the `package_manager` property.