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

fix(deps): update dependency p-limit to v4 - abandoned #181

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

renovate-bot
Copy link
Contributor

@renovate-bot renovate-bot commented Aug 13, 2021

Mend Renovate

This PR contains the following updates:

Package Change Age Adoption Passing Confidence
p-limit ^3.0.0 -> ^4.0.0 age adoption passing confidence

Configuration

📅 Schedule: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined).

🚦 Automerge: Disabled by config. Please merge this manually once you are satisfied.

Rebasing: Renovate will not automatically rebase this PR, because other commits have been found.

🔕 Ignore: Close this PR and you won't be reminded about this update again.


  • If you want to rebase/retry this PR, check this box

This PR has been generated by Mend Renovate. View repository job log here.

@amchiclet
Copy link
Contributor

I think there's a breaking change from p-limit v3 to v4. I'll probably need to manually create a PR for this upgrade.

@amchiclet
Copy link
Contributor

Here's what I've gathered after seeing p-limit's v4 release notes.

The upgrade from p-limit v3 to v4 is a breaking change such that it requires node 12 and above. Also, any module that depends on p-limit need to be pure ESM, (i.e. use import and not use require). Some details on how to work around is here.

The conflict with our current tests is:

  1. the new p-limit requires node 12.20 and above, while our tests still support node 10
  2. the typescript compiler output needs to be es2020, while it seems that busybench uses es2015

Some possible ways to move forward include:

  1. just not update p-limit and keep using v3
  2. conditionally use p-limit v3 and v4 depending on which test environment we're running
  3. not support node <v12
  4. not use p-limit

Given the team's resources, option 1 seems attractive at least for the next 1-2 release cycles (quarters).

Options 2 and 3 require making pprof-nodejs ESM-compatible. (Not entirely sure if this will affect current users who are not ESM-compatible the same way p-limit is affecting us or not.)

There's probably other options too.

@amchiclet
Copy link
Contributor

Discussed internally and the current direction is to drop support for node version under 12 and use p-limit v4.

@aabmass
Copy link
Member

aabmass commented Oct 24, 2022

Just dropped support for Node 12 so this should be unblocked #229

@aabmass aabmass self-assigned this Oct 24, 2022
@aabmass
Copy link
Member

aabmass commented Oct 24, 2022

The upgrade from p-limit v3 to v4 is a breaking change such that it requires node 12 and above. Also, any module that depends on p-limit need to be pure ESM, (i.e. use import and not use require). Some details on how to work around is here.

Actually, I think this will force us to ship this package as ESM only. Right now we are outputting CJS afaict. I don't think this is worth upgrading right now unless some security advisory comes up for 3.X

@forking-renovate
Copy link

forking-renovate bot commented Nov 28, 2022

Edited/Blocked Notification

Renovate will not automatically rebase this PR, because it does not recognize the last commit author and assumes somebody else may have edited the PR.

You can manually request rebase by checking the rebase/retry box above.

Warning: custom changes will be lost.

@renovate-bot renovate-bot changed the title fix(deps): update dependency p-limit to v4 fix(deps): update dependency p-limit to v4 - abandoned Nov 1, 2023
Copy link

Autoclosing Skipped

This PR has been flagged for autoclosing. However, it is being skipped due to the branch being already modified. Please close/delete it manually or report a bug if you think this is in error.

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

Successfully merging this pull request may close these issues.

7 participants