react-player icon indicating copy to clipboard operation
react-player copied to clipboard

Types wrong for ESM + module: Node16 / NodeNext

Open karlhorky opened this issue 2 years ago • 9 comments

Be sure to search for your issue before opening a new one.

Current Behavior

The types for [email protected] are incorrect when using ESM (eg. "type": "module" in package.json) and modern Node.js module resolution ("module": "Node16" or "module": "NodeNext" in tsconfig.json)

See the error on Are The Types Wrong?

The CJS module resolved at the package under contains a simulated export default with an __esModule marker, but no top-level module.exports. Node does not respect the __esModule marker, so accessing the intended default export will require a .default property access in Node from an ES module.

Screenshot 2023-04-16 at 18 17 56

You can also see this on this TypeScript Playground, with the code from the first example in the readme. Error message:

JSX element type 'ReactPlayer' does not have any construct or call signatures.(2604)

Expected Behavior

There should be no error

Steps to Reproduce

  1. Visit this TypeScript Playground
  2. See the error

Environment

  • URL attempting to play: --
  • Browser: --
  • Operating system: --
  • jsFiddle example: --

Other Information

--

karlhorky avatar Apr 16 '23 16:04 karlhorky

One thing that is working for me is to change all of the affected export default exports to use export = instead, similar to my PR for the @types/disposable-email-domains package.

Eg. for this export in types/file.d.ts:

https://github.com/cookpete/react-player/blob/662082a2f2edc863e8ca9cefdd7a3c88ad49ea0d/types/file.d.ts#L30

The change would look like this:

-export default class FilePlayer extends BaseReactPlayer<FilePlayerProps> {}
+export = class FilePlayer extends BaseReactPlayer<FilePlayerProps> {}

This is also compatible with projects using CommonJS and/or other module resolution types.

So probably not a huge PR, but probably all of the exports across all files need to change.

karlhorky avatar Apr 16 '23 16:04 karlhorky

This seems to be fixed https://arethetypeswrong.github.io/?p=react-player%402.16.0

luwes avatar Apr 20 '24 15:04 luwes

@luwes Thanks for the answer! It looks like Are The Types Wrong? does indeed report no errors for [email protected] now:

However, it seems that Are The Types Wrong? is not detecting the problem that (still) exists with [email protected] using the FilePlayer import:

https://github.com/cookpete/react-player/blob/795b19614977fbe2b89f6fd14503d1bfb121a722/src/players/FilePlayer.js#L20

The export default here (and on any other file that should be imported from) should be export =.

The current export default code leads to the following errors:

JSX element type 'ReactPlayer' does not have any construct or call signatures.ts(2604)

'ReactPlayer' cannot be used as a JSX component.
  Its type 'typeof import("/Users/k/p/courses/node_modules/react-player/file")' is not a valid JSX element type.ts(2786)

Screenshot 2024-04-20 at 17 30 02

Code:

import ReactPlayer from 'react-player/file.js';

type Props = {
  thumbnail: string;
  url: string;
};

export default function ResponsiveVideoPlayer(props: Props) {
  return <ReactPlayer {...props} />
}

Here's a TypeScript Playground to show the error:

(change the Module config to NodeNext under the TS Config tab - this is not saved in the URL below)

Fully-Specified Import Path

The fully-specified import path react-player/file.js is correct for Node16 and NodeNext module resolution.

cc @andrewbranch in case ATTW should also detect this problem

karlhorky avatar Apr 20 '24 15:04 karlhorky

how about https://arethetypeswrong.github.io/?p=react-player%403.0.0-canary.3/file https://www.npmjs.com/package/react-player/v/3.0.0-canary.3

luwes avatar Apr 20 '24 19:04 luwes

Looks like it's working without errors, yep!

Happy to either keep this open until a v3 non-canary release lands, or close it already now.

(I had to switch the import path to 'react-player/file' (no extension), because of the exports config in package.json)

// eslint-disable-next-line import/no-unresolved -- eslint-plugin-import doesn't understand `exports` in package.json yet https://github.com/import-js/eslint-plugin-import/issues/1810
import ReactPlayer from 'react-player/file';

type Props = {
  thumbnail: string;
  url: string;
};

export default function ResponsiveVideoPlayer(props: Props) {
  return <ReactPlayer {...props} />
}

Screenshot 2024-04-21 at 12 07 42

karlhorky avatar Apr 21 '24 10:04 karlhorky

Ah, seems like this 'react-player/file' import fails with a Default condition should be last one error message on compile time (Next.js) though 😬

Module not found: Default condition should be last one
  5 | // eslint-disable-next-line import/no-unresolved -- eslint-plugin-import doesn't understand `exports` in package.json yet https://github.com/import-js/eslint-plugin-import/issues/1810
> 6 | import ReactPlayer from 'react-player/file';
    | ^
  7 |
  8 | // ```
  9 |

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

So maybe the "exports" config in package.json is misconfigured...

Eg. an example PR fixing such an error:

  • https://github.com/firebase/firebase-js-sdk/pull/7007

I'll open a PR

karlhorky avatar Apr 21 '24 10:04 karlhorky

@luwes opened a PR to address the Default condition should be last one error above:

  • https://github.com/cookpete/react-player/pull/1828

karlhorky avatar Apr 21 '24 10:04 karlhorky

merged and released! https://www.npmjs.com/package/react-player/v/3.0.0-canary.4

here it seems to work but it's the latest Next.js https://codesandbox.io/p/devbox/react-player-ts-rdlt99?file=%2Fapp%2Fplayer.tsx%3A8%2C1

luwes avatar Apr 21 '24 13:04 luwes

Thanks for the review and merge!

I can confirm that [email protected] is working for our Next.js project 👍

karlhorky avatar Apr 21 '24 13:04 karlhorky