AzureAD / microsoft-authentication-library-for-js

Microsoft Authentication Library (MSAL) for JS
http://aka.ms/aadv2
MIT License
3.54k stars 2.62k forks source link

Incorrect type definitions for msal-browser and msal-react #6781

Open charliematters opened 7 months ago

charliematters commented 7 months ago

Core Library

MSAL.js (@azure/msal-browser)

Core Library Version

3.6.0

Wrapper Library

MSAL React (@azure/msal-react)

Wrapper Library Version

2.0.8

Public or Confidential Client?

Public

Description

We're using tshy to bundle a library which uses msal-browser and msal-react, and it is failing for type reasons:

The current file is a CommonJS module whose imports will produce 'require' calls; however, the referenced file is an ECMAScript module and cannot be imported with 'require'. Consider writing a dynamic 'import("@azure/msal-browser")' call instead.
  To convert this file to an ECMAScript module, change its file extension to '.mts' or create a local package.json file with `{ "type": "module" }`

The last time I asked the tshy author about this, they directed me to this page for checking type correctness, and it's flagging a few issues on both packages:

@azure/msal-browser

Problems 👺 Masquerading as ESM

Import resolved to an ESM type declaration file, but a CommonJS JavaScript file.

🥴 Internal resolution error

Import found in a type declaration file failed to resolve. Either this indicates that runtime resolution errors will occur, or (more likely) the types misrepresent the contents of the JavaScript files.

@azure/msal-react

Problems ⚠️ ESM (dynamic import only)

A require call resolved to an ESM JavaScript file, which is an error in Node and some bundlers. CommonJS consumers will need to use a dynamic import.

🥴 Internal resolution error

Import found in a type declaration file failed to resolve. Either this indicates that runtime resolution errors will occur, or (more likely) the types misrepresent the contents of the JavaScript files.

I notice that this issue is probably related: https://github.com/AzureAD/microsoft-authentication-library-for-js/issues/6377

Error Message

The current file is a CommonJS module whose imports will produce 'require' calls; however, the referenced file is an ECMAScript module and cannot be imported with 'require'. Consider writing a dynamic 'import("@azure/msal-browser")' call instead. To convert this file to an ECMAScript module, change its file extension to '.mts' or create a local package.json file with { "type": "module" }

Msal Logs

No response

MSAL Configuration

{}

Relevant Code Snippets

-

Reproduction Steps

-

Expected Behavior

No type errors, or at least allow compilation using tshy (which uses tsc under the hood)

Identity Provider

Azure AD / MSA

Browsers Affected (Select all that apply)

Other

Regression

No response

Source

External (Customer)

sameerag commented 3 months ago

@charliematters is this still seen?

charliematters commented 3 months ago

It looks like that type-checking website is still reporting the same errors, yes

koooge commented 2 months ago

Hi there, I encountered the same issue in @azure/msal-node@2.7.0. My project is commonjs using moduleResolution node16.

        "module": "node16",
        "moduleResolution": "node16",
        "esModuleInterop": true,

https://arethetypeswrong.github.io/?p=%40azure%2Fmsal-node%402.7.0

klippx commented 2 months ago

Problem

The problem is that you export this artefact ./dist/index.d.ts which contains e.g.

export type { UnauthenticatedTemplateProps } from "./components/UnauthenticatedTemplate";
export { useIsAuthenticated } from "./hooks/useIsAuthenticated";
export { useMsalAuthentication } from "./hooks/useMsalAuthentication";

The issue is not so bad, as it is just types and runtime things work anyway, but the issue can be for instance Typescript will not find the exports correctly and everything from your library will implicitly be any:

Screenshot 2024-05-15 at 17 02 54

Which can be a problem if you are a very strict project and do not allow any - for instance, people who know what they are doing and can be bothered to configure eslint with typescript introspection to discover these issues automatically.

Fix

The exported artefacts should use absolute imports. In the example above, it should rather look like this:

export type { AuthenticatedTemplateProps } from "./components/AuthenticatedTemplate.d.ts";
export { useIsAuthenticated } from "./hooks/useIsAuthenticated.js";
export { useMsalAuthentication } from "./hooks/useMsalAuthentication.js";

Of course, it is not only this file but ALL files in the dist/**/* folder that needs to follow this convention.

E.g in ./dist/hooks/useMsalAuthentication.d.ts:

import { AccountIdentifiers } from "../types/AccountIdentifiers.d.ts"; // <-- .d.ts extension added
klippx commented 2 months ago

My workaround right now is to enumerate all files that directly or indirectly import from any msal/* library, turning off the eslint rules.

Of course, I still do not get any Typescript support while coding, only any, but at least the project compiles.

module.exports = {
  overrides: [
    // Any file that directly or indirectly import from '@azure/msal-browser' / '@azure/msal-react'
    // must unfortunately turn off typescript-eslint, since type exports are not working as required.
    {
      files: [
        'src/components/Authentication.tsx',
        'src/components/GraphiQLContainer/plugins/Collection/useCollectionOperations.tsx',
        'src/components/GraphiQLContainer/useCopyQuery.tsx',
        'src/components/MsalProvider.tsx',
        'src/components/SideNav/ActiveLink.tsx',
        'src/components/SideNav/BottomAction.tsx',
        'src/components/SideNav/BottomActions.tsx',
        'src/components/SideNav/SideNavView.tsx',
        'src/components/TopBar/CurrentUser.tsx',
        'src/config/authConfig.ts',
        'src/utils/hooks/useAccessToken.tsx',
        'src/utils/hooks/useActiveAccount.tsx',
        'src/utils/trpc.ts',
      ],
      rules: {
        '@typescript-eslint/no-unsafe-argument': 'off',
        '@typescript-eslint/no-unsafe-assignment': 'off',
        '@typescript-eslint/no-unsafe-call': 'off',
        '@typescript-eslint/no-unsafe-member-access': 'off',
        '@typescript-eslint/no-unsafe-return': 'off',
      },
    },
  ],
  // ...
karlbohlmark commented 2 months ago

Another workaround seems to be to change moduleResolution to node10, but we would love to see this fixed.

juliangoacher commented 1 day ago

I came across this problem when setting up a react-admin auth provider and had to do the following to fix:

Would be great if the issues described above could be fixed though.