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: upgrade set-function-name to v2.0.1 #29

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

yisibl
Copy link

@yisibl yisibl commented Sep 14, 2023

There is a bug in set-function-name v2.0.0 and we should not use that version.

ljharb/set-function-name@db2eda8

@codecov
Copy link

codecov bot commented Sep 14, 2023

Codecov Report

Merging #29 (cadaf13) into main (ffb75fa) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main      #29   +/-   ##
=======================================
  Coverage   83.09%   83.09%           
=======================================
  Files           5        5           
  Lines          71       71           
  Branches       18       18           
=======================================
  Hits           59       59           
  Misses         12       12           

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@ljharb
Copy link
Member

ljharb commented Sep 14, 2023

This isn't required; we use ^, so you can just update your lockfile.

@ljharb ljharb marked this pull request as draft September 14, 2023 04:18
@yisibl
Copy link
Author

yisibl commented Sep 14, 2023

We're not currently using a lock file, and we're using an internal npm mirror, which can cause the app to fail to start once the version in the mirror is not the latest (version 2.0.0).

@ljharb
Copy link
Member

ljharb commented Sep 14, 2023

If you're not using a lockfile, then npm install $foo (where $foo is the direct dependency at the top of the chain that brings in set-function-name, should pull the latest version. Your npm mirror presumably automatically proxies to the public registry, so it should have v2.0.1 already.

@alar77
Copy link

alar77 commented Sep 14, 2023

This isn't required; we use ^, so you can just update your lockfile.

Maybe, but having an invalid requisite it's not optimal.
Basically, your package.json says Regexp.prototype.flags can work with set-function-name 2.0.0 but it's not true.
Imho fixing it is not optional

@yisibl
Copy link
Author

yisibl commented Sep 14, 2023

In general, I would expect that these packages should require better CI testing, and that npm publish would be automatically blocked until the CI test passes.

For example, here's a case of a missing dependency in set-function-name v2.0.0.

Or is there another better way?

@ljharb
Copy link
Member

ljharb commented Sep 14, 2023

@yisibl A missing explicit dependency doesn't necesasrily make tests fail, because dev deps, and transitive dev deps, are present in tests. The only way to check this is with eslint or a tool like depcheck, neither of which are yet in my standard toolkit since I've only made this mistake a handful of times out of tens of thousands of publishes going back a decade.

@alar77 it can, indeed, work with v2.0.0, so it is optional. It's just that if you have no other instances of functions-have-names in your dep graph that it won't work.

@TrevinAvery
Copy link

I am experiencing issues with this as well. I just installed a new package that was dependent on this, and it installed v2.0.0, causing me to deep dive to find the obscure issue. I believe this should be merged to fix the problem for future uers.

@ljharb
Copy link
Member

ljharb commented Oct 4, 2023

@TrevinAvery you must be using "not npm"? a working npm client installs the latest in-range version of transitive deps, not the lowest.

@TrevinAvery
Copy link

TrevinAvery commented Oct 4, 2023 via email

@ljharb
Copy link
Member

ljharb commented Oct 5, 2023

npm explain set-function-name, and then for each top-level dependency X that's implicated, npm install X.

@yisibl yisibl marked this pull request as ready for review October 7, 2023 06:56
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.

4 participants