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

fix: support esm named imports #425

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

prescottprue
Copy link

@prescottprue prescottprue commented Oct 22, 2024

PR Checklist

Please check if your PR fulfills the following requirements:

  • The commit message follows our guidelines
  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Adding Tests
  • Build related changes
  • CI related changes
  • Documentation changes
  • Other... Please describe:

What is the current behavior?

When using named imports in ESM you see the error saying that module.exports:

import { Client } from '@okta/okta-sdk-nodejs';
         ^^^^^^
SyntaxError: Named export 'Client' not found. The requested module '@okta/okta-sdk-nodejs' is a CommonJS module, which may not support all module.exports as named exports.
CommonJS modules can always be imported via the default export, for example using:

import pkg from '@okta/okta-sdk-nodejs';
const { Client } = pkg;

Though the default export mentioned in the message is technically possible, the need to do this isn't called out by tools like Typescript (more on this below)

Issue Number: N/A

What is the new behavior?

You can use named imports in ESM:

index.js

import { Client } from '@okta/okta-sdk-nodejs'
console.log('Client using named import', Client)

package.json

{
  "type": "module",
  "dependencies": {
    "@okta/okta-sdk-nodejs": "7.1.1"
  }
}

Running:

node ./index.js

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

Node making named exports via static analysis is part of a known issue in Typescript (i.e. the naming being available in Typescript but not in Node): https://www.typescriptlang.org/docs/handbook/modules/reference.html#interoperability-rules

Properties (other than default) of the CommonJS module’s module.exports may or may not be available as named imports to the ES module. Node.js attempts to make them available via static analysis. TypeScript cannot know from a declaration file whether that static analysis will succeed, and optimistically assumes it will. This limits TypeScript’s ability to catch named imports that may crash at runtime. See microsoft/TypeScript#54018 for more details.

As called out in this typescript issue:

Node uses cjs-module-lexer to syntactically analyze CommonJS modules without executing them in order to turn module.exports properties into named exports that can be imported as named (or namespace) imports by ES modules. However, syntactic analysis has its limitations, and not all module.exports properties get detected on all modules. (In practice, it’s common for this to be all-or-nothing, as with the typescript package in its current state: it has no named exports available.)

Reviewers

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

Successfully merging this pull request may close these issues.

1 participant