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

Provide a dynamic cjs wrapper built in to node-fetch that import(..) #1327

Open
jimmywarting opened this issue Oct 3, 2021 · 13 comments
Open
Labels

Comments

@jimmywarting
Copy link
Collaborator

jimmywarting commented Oct 3, 2021

Based on the documentation jimmywarting linked above, I created a helper typescript file that hopefully can serve other typescript users that are stuck:

// node-fetch.ts
// Helps importing node-fetch based on https://github.com/node-fetch/node-fetch/issues/1279

import type fetch from 'node-fetch';
export { RequestInit } from 'node-fetch'; // TODO: add more exports as needed here


// eslint-disable-next-line no-eval
const fetchPromise: Promise<typeof fetch> = eval('import("node-fetch")').then((mod: { default: typeof fetch }) => mod.default);
const exportedFetch: typeof fetch = (...args) => fetchPromise.then(fetch => fetch(...args));
export default exportedFetch;

Now, just import it with import fetch, { RequestInit } from './node-fetch'; almost as usual (note the ./)

It would be handy if such script was directly embedded in the package, like import fetch, { RequestInit } from 'node-fetch/cjs.js';.

Originally posted by @jeremyVignelles in #1266 (comment)


I think this is a good solution that dose not involve introducing a bundler and compiling everything into one file and importing all dependencies and basically duplicating everything. or using complex tools such as esbuild, noesm.
This dynamic import() have been suggested too many already and seems like good temporary easy solution.

I'm wondering if we should go ahead and introduce something like it built in to node-fetch. there is also the method such as lazy import only when needed: #1279 (comment) that i think is good solution also. it can have a faster boot up time. basically goes like this:

const _importDynamic = new Function('modulePath', 'return import(modulePath)')

module.exports = async function fetch(...args) {
  const {default: fetch} = await _importDynamic('node-fetch'))
  return fetch(...args))
}

One take a way from developers who uses a bundler/test framework such as jest, webpack, rollup, ts-node, ts-node-dev etc is that they like to rewrite the import() statement to require() or something like it. therefore i think a eval/new Function solution would be a good option

One thing to note doe is that it won't export Headers, Request and Response in a sync manner but most ppl only use the fetch method anyways(?) - would need to be documented that this dose not do the same thing

@jimmywarting jimmywarting changed the title Provide a cjs wrapper built in to node-fetch Provide a dynamic cjs wrapper built in to node-fetch that import(..)s Oct 3, 2021
@jimmywarting jimmywarting changed the title Provide a dynamic cjs wrapper built in to node-fetch that import(..)s Provide a dynamic cjs wrapper built in to node-fetch that import(..) Oct 3, 2021
@jeremyVignelles
Copy link

The eval method is an acceptable for me and would solve the friction users have upgrading to 3.x.

The only thing a developer using typescript would need to do is to replace

import fetch, { RequestInit } from 'node-fetch';

with

import fetch, { RequestInit } from 'node-fetch/cjs';

@jimmywarting
Copy link
Collaborator Author

jimmywarting commented Oct 3, 2021

@jeremyVignelles how about this instead?

// using type only imports (for typescript)
import type { RequestInit, Headers, Request, Response } from 'node-fetch';
import fetch from 'node-fetch/cjs';

@jeremyVignelles
Copy link

OK for me, though I'd prefer to keep a single import per dependency. The more dependency you have in a project, the longer the section becomes and it can be a nightmare to read...

@jimmywarting
Copy link
Collaborator Author

jimmywarting commented Oct 3, 2021

I'm also wondering about the naming if cjs is a good name for it. it can imply that it exports the same thing but with require instead... perhaps something like node-fetch/cjs-wrapper or something else would be better? don't really have any good name suggestions for it.

@jeremyVignelles
Copy link

jeremyVignelles commented Oct 3, 2021

it can imply that it exports the same thing

It does export the same thing, that can be used the same way. The way it's done internally is an implementation detail.

but with require instead

As a consumer, I'd tend to interpret this as a "I can import this with require()" (We just need to make sure that it actually can 😄 ). As a consumer not knowing the difference between cjs and mjs, I'd just do what I'm told in the README anyway... That said, I'm ok with cjs-wrapper too

@jimmywarting
Copy link
Collaborator Author

jimmywarting commented Oct 3, 2021

It does export the same thing

Well, yes but only the fetch method... not the rest, like the other classes

@jeremyVignelles
Copy link

jeremyVignelles commented Oct 3, 2021

Indeed, the classes cannot be imported that way synchronously...

What about something like:

export const RequestPromise: Promise<typeof Request> = fetchPromise.then(module => module.Request);

imported like:

import fetch, { RequestPromise } from 'node-fetch/cjs';

// Later
const Request = await RequestPromise;

The usage differs a lot now. Not sure if there's a better workaround available...

@jimmywarting
Copy link
Collaborator Author

// Later
const Request = await RequestPromise;

cjs can't use top level await... if they do use await then they can just as well use everything = await import('node-fetch')

@jeremyVignelles
Copy link

That's what I meant by later, it's inside an async function...

they can just as well use everything = await import('node-fetch')

Except for typescript where it's translated into a require() call, which is what we want to avoid with this wrapper...

There should be a way to access the classes, but other than the workaround I suggested (using the Promise<Request>), I don't see any other workaround... Not sure it does even work...

@Donnevtis
Copy link

Some trouble. It's a mess. I can't use ES modules in a serverless environment. The typescript compiles a dynamic import to require. @types/node-fetch is deprecated. What do you want me to do?

@LinusU
Copy link
Member

LinusU commented Oct 9, 2021

Except for typescript where it's translated into a require() call, which is what we want to avoid with this wrapper...

This is fixed in the latest TypeScript beta, which should be out in ~1 month 🎉

https://devblogs.microsoft.com/typescript/announcing-typescript-4-5-beta/

@jimmywarting
Copy link
Collaborator Author

still wondering about what we should be doing about this issue, would like to hear someones tough if this should be done before working on v4

@jeremyVignelles
Copy link

The fact that typescript postponed the feature is disappointing, and we're stuck in the same mess as before.

I'm not a fan of needing an await for importing classes, but user-made workarounds will have the same issue, and the classes are used in few use cases, right?

I'd embed the workaround with proper warnings and in a separate file. Users must opt-in for the workaround until it is fixed by typescript.

What else could we do?

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

No branches or pull requests

4 participants