snowflakedb / snowflake-connector-nodejs

NodeJS driver
Apache License 2.0
123 stars 130 forks source link

SNOW-889050: NextJS Weird Warning Message #609

Closed detailedghost closed 1 year ago

detailedghost commented 1 year ago

Summary

Working with an NX monorepo with NextJS. I'm able to query the database, but grabbing a weird warning message when I launch the application. Figured it might be worth flagging, sorry if there's a duplicate issue. Regardless, I appreciate any response.

Requested info

  1. What version of NodeJS driver are you using?
    • v18.16.0
  2. What operating system and processor architecture are you using?
    • Windows 10 with WSL 2 Ubuntu
  3. What version of NodeJS are you using?
    • v18.16.0
  4. What are the component versions in the environment (npm list)?
    ├── @nx/cypress@16.6.0
    ├── @nx/eslint-plugin@16.6.0
    ├── @nx/jest@16.6.0
    ├── @nx/js@16.6.0
    ├── @nx/linter@16.5.5
    ├── @nx/next@16.6.0
    ├── @nx/react@16.5.5
    ├── @nx/workspace@16.6.0
    ├── @radix-ui/react-navigation-menu@1.1.3
    ├── @radix-ui/react-slot@1.0.2
    ├── @swc/cli@0.1.62
    ├── @swc/core@1.3.75
    ├── @swc/helpers@0.5.1
    ├── @tanstack/react-query@4.32.6
    ├── @testing-library/react@14.0.0
    ├── @types/jest@29.5.3
    ├── @types/node-fetch@2.6.4
    ├── @types/node@20.4.8
    ├── @types/react-dom@18.2.7
    ├── @types/react@18.2.18
    ├── @types/snowflake-sdk@1.6.13
    ├── @typescript-eslint/eslint-plugin@5.62.0
    ├── @typescript-eslint/parser@5.62.0
    ├── autoprefixer@10.4.14
    ├── babel-jest@29.6.2
    ├── class-variance-authority@0.7.0
    ├── clsx@2.0.0
    ├── cypress@4.12.1
    ├── eslint-config-next@13.4.12
    ├── eslint-config-prettier@8.1.0
    ├── eslint-plugin-cypress@2.14.0
    ├── eslint-plugin-import@2.28.0
    ├── eslint-plugin-jsx-a11y@6.7.1
    ├── eslint-plugin-react-hooks@4.6.0
    ├── eslint-plugin-react@7.32.2
    ├── eslint@8.46.0
    ├── jest-environment-jsdom@29.6.2
    ├── jest-environment-node@29.6.2
    ├── jest@29.6.2
    ├── lucide-react@0.263.1
    ├── next@13.4.1
    ├── node-fetch@3.3.2
    ├── nx-cloud@16.2.0
    ├── nx@16.5.5
    ├── postcss@8.4.27
    ├── prettier@3.0.1
    ├── prisma@5.1.1
    ├── proxy-agent@5.0.0
    ├── react-dom@18.2.0
    ├── react-hook-form@7.45.4
    ├── react@18.2.0
    ├── snowflake-sdk@1.7.0
    ├── tailwind-merge@1.14.0
    ├── tailwindcss-animate@1.0.6
    ├── tailwindcss@3.3.3
    ├── ts-jest@29.1.1
    ├── ts-node@10.9.1
    ├── tslib@2.6.1
    ├── typescript@5.1.6
    └── zod@3.21.4

7.Server version:*

  1. What did you do?

    • Install the snowflake-sdk
    • Connected and queried a test database
  2. What did you expect to see?

    • No warning messages when launching NextJs app
  3. What should have happened and what happened instead?

    • No strange NextJS warning messages
  4. Can you set logging to DEBUG and collect the logs?

  5. What is your Snowflake account identifier, if any? (Optional)

    • SLNASYY-KI87678
sfc-gh-dszmolka commented 1 year ago

hi and thank you for raising this issue. it looks like similar issues are all over the place, for example

and so on, even outside of snowflake-sdk. Seems to be originating from encoding being an optional dependency for node-fetch (which latter module is an indirect dependency for snowflake-sdk) and as you mentioned, is only a warning message and does not cause any actual issues.

as a workaround for now, it should go away if you install encoding with npm i -D encoding (or npm i encoding). I'll also check how we could get rid of it without needing the workaround.

sfc-gh-dszmolka commented 1 year ago

before trying to solve this issue in the snowflake-sdk package, I wanted to see it myself that adding encoding to the dev-dependencies will work. (since node-fetch v2 requires it as peer-dependency but nobody provides it)

i don't have much experience with Next.JS, so following the docs I started setting up a new Next.js project from scratch, excluded some node-only stuff from webpack config in next.config.js (fs, child_process, tls, net, dns, async_hooks, module) and after I got past this phase, I faced the following error:

Module not found: Can't resolve 'proxy-agent'

https://nextjs.org/docs/messages/module-not-found

Import trace for requested module:
./node_modules/urllib/lib/urllib.js
./node_modules/urllib/lib/index.js
./node_modules/snowflake-sdk/lib/http/base.js
./node_modules/snowflake-sdk/lib/http/node.js
./node_modules/snowflake-sdk/lib/snowflake.js
./node_modules/snowflake-sdk/index.js
./pages/snowflake.js

which is a different module missing from one of the dependencies of snowflake-sdk, but exactly the same situation as I see it: similarly to how node-fetch v2 requires encoding as a peer-dependency; so does urllib require proxy-agent as a peer-dependency:

  "peerDependencies": {
    "proxy-agent": "^5.0.0"
  },
  "peerDependenciesMeta": {
    "proxy-agent": {
      "optional": true
    }
  },

and in both cases, nobody provides this to them (encoding to node-fetch v2, and proxy-agent to urllib).

Since situation seems to be very similar, but with different modules, I'm wondering how is the missing proxy-agent problem solved in your Next.js setup today? Since it doesn't seem to be missing in your case. I'm wondering , why isn't it missing and how is encoding different which is also a peer-dependency for another module (and not provided by anything).

sfc-gh-dszmolka commented 1 year ago

okay I think I found something. apparently just very recently urllib released a fresh release 2.41.0 to fix a critical vulnerability, which was caused by one of their indirect dependencies, vm2 (needed by proxy-agent)

the dev over there moved proxy-agent (which introduces vm2 which has a critical, unfixed vulnerability) into peer-dependencies - and at the same time, they provide proxy-agent it as dev-dependency. see PR#457 in https://github.com/node-modules/urllib

before we could move on with a similar approach, I really would like to see it working, before we introduce a new dev- dependency into our module which will be probably unused anyways - to get rid of a warning.

Do you think it would be possible to provide a minimal viable reproduction Next.Js app, which when run, exhibits the error that only encoding is missing ?

detailedghost commented 1 year ago

@sfc-gh-dszmolka Thanks for the investigation. The good news is that the package does work, I'm able to query the database just fine, just the weird warn message is spit out at the beginning.

I took the dumb approach and just installed "proxy-agent" and that seemed to get me a simple warning message. Doing a regular next app seems to break down, and complain about the proxy agent. I can go down the rabbit hole and try to manually install every needed dependency but certainly isn't ideal. Is there a flag to hide the warn message?

sfc-gh-dszmolka commented 1 year ago

i'm not entirely sure about what options are in next.js to hide the warning messages, but some web search showed these hits:

hope some of them helps. the issue seems to be that some direct or indirect dependencies put certain packages in their peer-dependencies

urllib seems to satisfy it for itself in dev-dependencies, but node-fetch does not from anywhere. Therefore the workaround seems to be either to install these packages as dev-dependency (as it worked for multiple people, see Issues from the original response), or -and i did not test this- try and suppress the warning or configure the fallback. Again, I'm not very familiar with Next.JS but hope this makes sense. Perhaps there are other ways too to hide this warning message in Next.JS. There is no such issue in Node.JS for which this driver is built and tested against.

detailedghost commented 1 year ago

Wanted to follow back up. Still exhibiting the warning message, even with adding in encoding, proxy-agent, coffee-script and a few others in as dev dependencies.

Also tried route 2 with attempting to suppress the issue via next.config.js as suggested, but it appears the import error message is outside the scope of NextJS.

I did notice that vm2 lib flags users to migrate to isolated-vm. Could that possibly be the issue?

sfc-gh-dszmolka commented 1 year ago

I'm really not sure anymore if we should address this Next.JS related issue here. This connector is built for and tested on Node.JS - and there's no such issues on Node.JS

  1. For the original issue (missing encoding emits a warning), please consider filing an issue against node-fetch the warning comes from there. (encoding peer-depended on optionally, but not provided)

  2. For the newer issue (missing stuff like proxy-agent et. al), it comes from urllib v2, since they moved vulnerable vm2 to peer-dependencies. This solves a critical security issue for a lot of users (CVE-2023-37466, CVE-2023-37903), but seems to provide a console-emitted warning for you. You can just ignore these warnings. In the long term, they should automatically be gone once we manage to move on to urllib v3, because it does not depend on proxy-agent.

Since this issue does not exist in Node.JS, I'm marking this one as closed but if anyone stumbles upon it and has a solution which can make the harmless warnings disappear in Next.JS, they can share it in a comment.