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

"Can't resolve encoding" warning in the console when used with Next JS 13.4 #7280

Closed
raajnadar opened this issue May 7, 2023 · 22 comments
Closed
Assignees

Comments

@raajnadar
Copy link
Contributor

[REQUIRED] Describe your environment

  • Operating System version: Mac OS X
  • Browser version: latest Chromium
  • Firebase SDK version: 9.21.0
  • Firebase Product: auth (auth, database, storage, etc)

[REQUIRED] Describe the problem

Steps to reproduce:

You need to import the package in a newly created next js application & run the application in the dev

- warn ../node_modules/node-fetch/lib/index.js
Module not found: Can't resolve 'encoding' in '<path>/node_modules/node-fetch/lib'
 
Import trace for requested module:
../node_modules/node-fetch/lib/index.js
../node_modules/@firebase/auth/dist/node-esm/index.js
../node_modules/firebase/auth/dist/index.mjs
./context/AuthProvider.tsx
./app/root-layout.tsx

Relevant Code:

import { getAuth } from 'firebase/auth'

export const auth = getAuth()

The solution is to use the new version of node fetch in https://github.com/firebase/firebase-js-sdk/blob/master/packages/auth/package.json

@jbalidiong
Copy link
Contributor

Hi @raajnadar, thanks for reaching out. I was able to replicate the behavior that shows the warning when using Next JS 13.4. Let me check what we can do for this issue or bring someone from the Auth team that can provide more context about it. I’ll update this thread if I have any information to share.

@jbalidiong jbalidiong added the bug label May 8, 2023
@prameshj
Copy link
Contributor

prameshj commented May 8, 2023

There is a WIP to update the node-fetch version, but it has some CI failures which needs investigation - #5476

Can you clarify which node-fetch version resolved the issue for you? Thanks!

@raajnadar
Copy link
Contributor Author

@prameshj in the CI logs it seems there are typescript errors which can be resolved easily

@raajnadar
Copy link
Contributor Author

Can this be added to the package?

https://stackoverflow.com/a/76092251/5519872

@matamatanot
Copy link

matamatanot commented May 12, 2023

@prameshj

https://nextjs.org/docs/architecture/supported-browsers#polyfills
https://www.npmjs.com/package/whatwg-fetch

Next.js replaces fetch with whatwg-fetch
and

In addition, to reduce bundle size, Next.js will only load these polyfills for browsers that require them. The majority of the web traffic globally will not download these polyfills.

If necessary, dynamically importing node-fetch is preferable since fetch is already available in many default environments.

@dwyfrequency dwyfrequency self-assigned this May 15, 2023
@hsubox76
Copy link
Contributor

We will try to upgrade node-fetch to v3 if possible. It looks like this will not be fixed in any version of node-fetch v2: node-fetch/node-fetch#675

Dynamic imports would be a larger change, we have avoided including dynamic import statements in the SDK for a while because not all environments support them. It may be time to revisit this as support has increased a lot in the last few years. It still doesn't work in Node 12 and below so at the very least we'll have to bump the latest version of Node we support and put it somewhere in the docs. We'll need to do a thorough review to explore what other unusual environments might not be covered, e.g. React Native, browser extensions, incognito/private, service workers.

@hsubox76 hsubox76 changed the title Some warning in the console when used with Next JS 13.4 "Can't resolve encoding" warning in the console when used with Next JS 13.4 Jun 16, 2023
@hsubox76
Copy link
Contributor

We're still working on updating node-fetch to version 3 as described in the above comment, for what it's worth this issue in the Next.js repo says it's just a warning and it shouldn't break anything: vercel/next.js#7621

@raajnadar
Copy link
Contributor Author

raajnadar commented Jun 18, 2023

I totaly agree with you, but this warning is spammed on terminal which makes it difficult to read logs on dev mode

@DellaBitta
Copy link
Contributor

Would the installation of encoding be a work around to prevent the logged warning for now?

@balazsorban44
Copy link

balazsorban44 commented Aug 15, 2023

@hsubox76 I recommend simply not including node-fetch anymore. Node 16 is EOL soon, and all Node.js versions above have globalThis.fetch available by default. See https://nodejs.org/api/globals.html#fetch

FWIW, Next.js also polyfills fetch for lower versions, using undici, which is the same package that Node.js uses in their native globalThis.fetch implementation. To match runtimes/increase interoperability, if you absolutely want to patch fetch, I recommend using undici instead. This could be done in a patch, in contrast to dropping node-fetch which might be a breaking change. Ideally, you would include this conditionally, only if typeof globalThis.fetch === "undefined"

@hsubox76
Copy link
Contributor

Thanks for the info! We haven't specified a minimum version of Node we support, but we're working on a policy, hopefully tied to Node's own EOL schedule. I think the highest minimum explicitly set right now is 10 on Firestore so we may have users that still expect 10 or at least 12 to work even though we haven't made any official guarantees.

Dynamic imports are something I'm hesitant about for the reasons above, but maybe we could import undici in all cases and only call it if globalThis.fetch is undefined. I think users' bundlers should then tree-shake it out if it's not used, if they are using tree-shaking.

@matamatanot
Copy link

next.js will remove node-fetch

vercel/next.js#55112

@balazsorban44
Copy link

balazsorban44 commented Sep 8, 2023

@matamatanot it has already been removed 4 months ago, in favor of undici/platform default. That PR is only a cleanup in the repo.

@dedeard
Copy link

dedeard commented Sep 9, 2023

It makes me happy npm install encoding.

@matamatanot
Copy link

nodejs/node#45684

'fetch' is now stable!

@landsman
Copy link

Time to fix this guys 🙏

@hsubox76
Copy link
Contributor

We're actively working on this. The challenges we're running into are:

  1. node-fetch version 3 is ESM only, and doesn't work well alongside CJS dependencies, which we have (e.g. mocha)
  2. There is a wrapper package that wraps node-fetch version 3 so it can be usable with CJS: node-fetch-cjs but we're having some problems with that as well
  3. Native fetch is stable in 18 and 16 is EOL so that sounds simple on the surface, but the latest Node download stats show only 51% of users are on Node versions >= 18, and as we don't really have a published minimum, it might surprise some users to find 16 and below suddenly not supported.
  4. We have constraints that prevent us from using dynamic imports as a solution (e.g., only importing node-fetch if it can't find native fetch)

Given that, we're currently taking a shot at seeing if we can migrating undici, but even in the best case scenario it will require some minor code changes and testing.

node-fetch is currently used in Auth, Firestore, Functions, Storage, and rules-unit-testing, as well as some of our CI, so we need to make the solution (hopefully undici?) work across all of these and make sure it doesn't break anything. Any suggestions for alternative solutions would also be welcome, given the constraints above.

@landsman
Copy link

  1. is there a chance to drop CJS and rewrite it to ESM? It's a quite legacy today.
  2. overkill
  3. nevermind, just update and motivate people to upgrade too, this has to be fast... when people do not have reason to update, they just don't
  4. dynamic import is quite a good way, that's a pitty

btw why do you have to use fetch anyway? there are alternatives, right?

@hsubox76
Copy link
Contributor

It's not an issue of our own code being in CJS. We export both CJS and ESM bundles. Most of our products that support both browser and Node have at least 4 bundles - browser CJS (Jest/JSDOM requires this), browser ESM, Node CJS, Node ESM. As a library, we have to support a broad range of developers, including those that can't update to the latest technology due to various reasons, such as having required legacy tools and dependencies that are not yet updated.

Firebase is used by all kinds of developers on all kinds of platforms, including browsers, browser extensions, service workers, Node, edge, native webview (Cordova/Ionic), React Native, Flutter, Electron, smart TV OS'es, in greenfield apps and in large legacy codebases mixed in with other many other libraries. We do our best to support developers in as many cases as we can, and this sometimes prevents us from being able to use all the cutting edge technology and telling developers who aren't able to upgrade, or are on the wrong platform, to just deal with it. We obviously can't support every environment forever (we recently dropped IE) but we have to walk a fine line between reasonably dropping support for very old, unused environments and just leaving 50% of our Node users with broken apps suddenly.

Also, as I mentioned above, we're trying undici as an alternative to node-fetch, and hopefully we can get it to work.

@cvandradg
Copy link

any update on this?

@DellaBitta
Copy link
Contributor

We merged a PR to upgrade the older node-fetch dependency to the latest version of undici in all but our rules-unit-testing SDK. This change should be in our next release (v10.7.0).

@DellaBitta
Copy link
Contributor

v10.7.0 with the aforementioned change to undici has been released. I'm going to close this issue for now. If you still have problems related to this ticket then please open a new ticket and link to this one. Thanks!

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

Successfully merging a pull request may close this issue.