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

Types wrong for ESM + module: Node16 / NodeNext #1613

Open
karlhorky opened this issue Apr 16, 2023 · 9 comments
Open

Types wrong for ESM + module: Node16 / NodeNext #1613

karlhorky opened this issue Apr 16, 2023 · 9 comments

Comments

@karlhorky
Copy link

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
Copy link
Author

karlhorky commented Apr 16, 2023

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:

export default class FilePlayer extends BaseReactPlayer<FilePlayerProps> {}

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.

@luwes luwes added the bug Indicates an unexpected problem or unintended behavior label Oct 10, 2023
@luwes
Copy link
Collaborator

luwes commented Apr 20, 2024

@luwes luwes closed this as completed Apr 20, 2024
@karlhorky
Copy link
Author

karlhorky commented Apr 20, 2024

@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:

export default class FilePlayer extends Component {

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

@luwes luwes reopened this Apr 20, 2024
@luwes
Copy link
Collaborator

luwes commented Apr 20, 2024

@karlhorky
Copy link
Author

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
Copy link
Author

karlhorky commented Apr 21, 2024

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:

I'll open a PR

@karlhorky
Copy link
Author

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

@luwes
Copy link
Collaborator

luwes commented Apr 21, 2024

@karlhorky
Copy link
Author

Thanks for the review and merge!

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

@luwes luwes removed the bug Indicates an unexpected problem or unintended behavior label Apr 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants