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

add option to specify package manager #77

Open
phryneas opened this issue Apr 17, 2022 · 16 comments
Open

add option to specify package manager #77

phryneas opened this issue Apr 17, 2022 · 16 comments

Comments

@phryneas
Copy link

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
Copy link

zchenwei commented May 3, 2022

+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
Copy link

soanvig commented May 3, 2022

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
Copy link

zchenwei commented May 3, 2022

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
Copy link

soanvig commented May 4, 2022

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

@markerikson
Copy link

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
Copy link
Owner

andresz1 commented May 30, 2022

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
Copy link

@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
Copy link
Owner

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

@andresz1
Copy link
Owner

andresz1 commented May 30, 2022

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

@markerikson
Copy link

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
Copy link
Owner

andresz1 commented May 30, 2022

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
Copy link

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 reduxjs/redux-toolkit#2374 .

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

@andresz1
Copy link
Owner

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

@zchenwei
Copy link

zchenwei commented Jun 6, 2022

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
Copy link
Contributor

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
Copy link

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

7 participants