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

SNOW-897216: Package should declare dependency on asn1.js #624

Closed
ajwootto opened this issue Aug 21, 2023 · 12 comments
Closed

SNOW-897216: Package should declare dependency on asn1.js #624

ajwootto opened this issue Aug 21, 2023 · 12 comments
Assignees
Labels
bug Something isn't working status-fixed_awaiting_release The issue has been fixed, its PR merged, and now awaiting the next release cycle of the connector.

Comments

@ajwootto
Copy link

ajwootto commented Aug 21, 2023

  1. What version of NodeJS driver are you using?
    1.7.0

  2. What operating system and processor architecture are you using?

    Mac OS Apple Silicon

  3. What version of NodeJS are you using?
    (node --version and npm --version)
    18.16

Installing this package using a stricter package manager such as pnpm or yarn in pnpm mode will result in an error from the
asn1.js-rfc2560 dependency when it attempts to resolve asn1.js. This is because the asn1.js-rfc2560 package declares asn1.js as a peerDependency, but this package does not supply it with one. The reason this works fine in regular npm installs is because the other asn1-related dependency this package asks for (asn1.js-rfc5280) declares a direct dependency on asn1.js. It is then installed, and with npm it gets "hoisted" to the root of node_modules/ allowing the other package with the peer dependency to find it. pnpm does not allow this type of hoisting, so this package needs to declare a direct dependency on asn1.js in order to work.

@ajwootto ajwootto added the bug Something isn't working label Aug 21, 2023
@github-actions github-actions bot changed the title Package should declare dependency on asn1.js SNOW-897216: Package should declare dependency on asn1.js Aug 21, 2023
@sfc-gh-dszmolka sfc-gh-dszmolka self-assigned this Aug 22, 2023
@sfc-gh-dszmolka sfc-gh-dszmolka added the status-triage Issue is under initial triage label Aug 22, 2023
@sfc-gh-dszmolka
Copy link
Collaborator

hi and thank you for raising this issue and appreciate the detailed explanation!

i wanted to see for myself but I'm obviously missing something, because I could not reproduce in a node:18 container.
steps I've taken:

  1. install pnpm
  2. install snowflake-sdk
# node -v
v18.16.1
# curl -fsSL https://get.pnpm.io/install.sh | bash -
==> Downloading pnpm binaries 8.6.12
..gets installed..

# pnpm install snowflake-sdk    
 WARN  deprecated [email protected]: Please upgrade  to version 7 or higher.  Older versions may use Math.random() in certain circumstances, which is known to be problematic.  See https://v8.dev/blog/math-random for details.1.0: 16.38 kB/10.87 MB
 WARN  deprecated [email protected]: Package no longer supported. Contact Support at https://www.npmjs.com/support for more info.
 WARN  deprecated [email protected]: The querystring API is considered Legacy. new code should use the URLSearchParams API instead.
Packages: +221
+++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
Packages are hard linked from the content-addressable store to the virtual store.
  Content-addressable store is at: /root/.local/share/pnpm/store/v3
  Virtual store is at:             node_modules/.pnpm
Progress: resolved 221, reused 0, downloaded 221, added 221, done

dependencies:
+ snowflake-sdk 1.7.0

Downloading registry.npmjs.org/aws-sdk/2.1441.0: 10.87 MB/10.87 MB, done
Done in 7.8s

# pnpm why asn1.js
Legend: production dependency, optional only, dev only

/node

dependencies:
snowflake-sdk 1.7.0
├─┬ @techteamer/ocsp 1.0.0
│ ├── asn1.js 5.4.1
│ ├─┬ asn1.js-rfc2560 5.0.1
│ │ ├── asn1.js 5.4.1 peer
│ │ └─┬ asn1.js-rfc5280 3.0.0
│ │   └── asn1.js 5.4.1
│ └─┬ asn1.js-rfc5280 3.0.0
│   └── asn1.js 5.4.1
├─┬ asn1.js-rfc2560 5.0.1
│ ├── asn1.js 5.4.1 peer
│ └─┬ asn1.js-rfc5280 3.0.0
│   └── asn1.js 5.4.1
└─┬ asn1.js-rfc5280 3.0.0
  └── asn1.js 5.4.1

even with pnpm config set strict-peer-dependencies true , snowflake-sdk gets installed correctly.
with our without the above setting, after installation, i'm able to run the example script to connect to SF and fetch data.

can you perhaps provide the reproduction steps or what other setting i need to look at ? thank you in advance !

@sfc-gh-dszmolka sfc-gh-dszmolka added the status-information_needed Additional information is required from the reporter label Aug 22, 2023
@ajwootto
Copy link
Author

Hi @sfc-gh-dszmolka thanks for taking a look!

In my original post I said that using pnpm would trigger the issue, but I was actually using yarn with the nodeLinker option set to pnpm. I assumed the two things would be equivalent but it appears they are not. Sorry for the confusion.

My actual repro steps are.

  1. Create a new project
  2. ensure yarn is using the latest 3.x version with yarn set version stable
  3. change or create a .yarnrc.yml file with the line nodeLinker: pnpm
  4. run yarn install
  5. check node_modules/ folder

This is what I see in the node_modules directory afterwards:
image
Notably the asn1.js package is missing from the node_modules/ of asn1.js-rfc2560 and also does not appear at the top-level node_modules/

I'm actually unsure at this point whether this would be considered a bug in yarn itself, though it does seem intuitively correct to me that it shouldn't satisfy a peer requirement of one package with a subdependency of another

@sfc-gh-dszmolka
Copy link
Collaborator

thank you @ajwootto ; makes sense and of course thanks to your repro, it easily reproduces for me as well. As an interim solution, I could avoid the warning (and also, get asn1.js installed) by simply adding it to the devDependencies of snowflake-sdk:

  "devDependencies": {
    "asn1.js": "^5.0.0",   <<< new line added
    "async": "^3.2.3",

perhaps it can be used as a workaround. Anyways, we'll take a look.

@sfc-gh-dszmolka sfc-gh-dszmolka added status-in_progress Issue is worked on by the driver team and removed status-triage Issue is under initial triage status-information_needed Additional information is required from the reporter labels Aug 23, 2023
@ajwootto
Copy link
Author

@sfc-gh-dszmolka glad you could get a repro!

I don't think adding it to devDependencies will fix the issue though since as far as I know it's being used at runtime, so I think it needs to go in dependencies.

@sfc-gh-dszmolka
Copy link
Collaborator

well, for me adding asn1.js to devDependencies and using your repro setup, did two things:

  • get rid of the warning of course
  • asn1.js was installed into the node_modules directory:
#  ls -la node_modules/asn*
lrwxrwxrwx 1 root root 56 Aug 23 06:49 node_modules/asn1.js -> .store/asn1.js-npm-5.4.1-37c7edbcb0/node_modules/asn1.js
lrwxrwxrwx 1 root root 70 Aug 23 06:49 node_modules/asn1.js-rfc2560 -> .store/asn1.js-rfc2560-virtual-6cae34e700/node_modules/asn1.js-rfc2560
lrwxrwxrwx 1 root root 72 Aug 23 06:49 node_modules/asn1.js-rfc5280 -> .store/asn1.js-rfc5280-npm-3.0.0-5b944d0cac/node_modules/asn1.js-rfc5280

we're discussing this internally how we would like to address this behaviour and I'll keep this Issue updated on the outcome.

@HarlemSquirrel
Copy link

I was able to work around this by adding the following to my .yarnrc.yml

packageExtensions:
  snowflake-sdk@*:
    dependencies:
      "asn1.js": "^5.0.0" # https://github.com/snowflakedb/snowflake-connector-nodejs/issues/624

@sfc-gh-dszmolka
Copy link
Collaborator

PR with the fix: #654

@sfc-gh-dszmolka sfc-gh-dszmolka added status-pr_pending_merge A PR is made and is under review and removed status-in_progress Issue is worked on by the driver team labels Sep 29, 2023
@sfc-gh-dszmolka
Copy link
Collaborator

PR is merged and will be part of the October release, expected towards end of October.
We ended up going with the approach of peer-depending asn1.js, which eliminates the error described in this Issue, confirmed with re-running the reproduction pre and post change.

@sfc-gh-dszmolka sfc-gh-dszmolka added status-fixed_awaiting_release The issue has been fixed, its PR merged, and now awaiting the next release cycle of the connector. and removed status-pr_pending_merge A PR is made and is under review labels Sep 30, 2023
@sfc-gh-dszmolka
Copy link
Collaborator

node.js driver version 1.9.1 released with the fix and is available on npm. thank you all for bearing with us !

@HarlemSquirrel
Copy link

I'm still seeing this issue with the version 1.9.1 when I remove my above workaround.

yarn install
➤ YN0000: ┌ Resolution step
➤ YN0002: │ my-proj@workspace:. doesn't provide asn1.js (p2c9f1), requested by snowflake-sdk
➤ YN0000: │ Some peer dependencies are incorrectly met; run yarn explain peer-requirements <hash> for details, where <hash> is the six-letter p-prefixed code
➤ YN0000: └ Completed
➤ YN0000: ┌ Fetch step
➤ YN0000: └ Completed in 0s 205ms
➤ YN0000: ┌ Link step
➤ YN0000: └ Completed in 0s 286ms
➤ YN0000: Done with warnings in 0s 809ms

yarn explain peer-requirements p2c9f1
➤ YN0000: my-proj@workspace:. doesn't provide asn1.js, breaking the following requirements:

➤ YN0000: asn1.js-rfc2560@npm:5.0.1 [88ff7] → ^5.0.0 ✘
➤ YN0000: snowflake-sdk@npm:1.9.1 [9dccf]   → ^5.4.1 ✘

➤ YN0000: Note: these requirements start with snowflake-sdk@npm:1.9.1 [9dccf]

@sfc-gh-dszmolka
Copy link
Collaborator

the snowflake-sdk package declares peerDependency on asn1.js , as mentioned in previous comments

We ended up going with the approach of peer-depending asn1.js, which eliminates the error described in this Issue, confirmed with re-running the reproduction pre and post change.

meaning after a long discussion (as i remember it) the decision was to rely on the project which uses snowflake-sdk as a dependency, the same project to provide the runtime environment with asn1.js

@HarlemSquirrel
Copy link

So the installation instructions should be updated to include this as well?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working status-fixed_awaiting_release The issue has been fixed, its PR merged, and now awaiting the next release cycle of the connector.
Projects
None yet
Development

No branches or pull requests

4 participants