"adal" package

[ghost]

• I want to talk about adal/index.d.ts.
  • The authors of that type definition are cc/ @mmaitre314

This does declare module 'adal' but npm info adal turns up nothing. Did you mean "adal-node"?
(If so, we should deprecate this and move to @types/adal-node.)

Also, if you want adal-angular published it should be in its own directory or listed in the tsconfig.

[craxal]

I agree. The types package name needs to match the npm package name.

[mmaitre314]

The definition matches the structure of the adal-angular package. There is no 'adal' package.
https://github.com/AzureAD/azure-activedirectory-library-for-js

[mmaitre314]

Is that what breaks typing install?
AzureAD/azure-activedirectory-library-for-js#61

Splitting the files should be simple but it is weird to have the type package not match the npm package.

[ghost]

Looks like this was called adal-angular on master but got changed to adal during conversion.
4f7524f

I assume we have no typings for "adal-node" at the moment.

If we're really just defining adal-angular, we should have a single file adal-angular/index.d.ts, and not have declare module "adal". @mmaitre314, if this is correct, could you make a PR for this and see if it still works with your code? Then I'll deprecate the @types/adal package as being renamed to @types/adal-angular.

[maitre-matt]

[mmaitre314] A single type file would break ADAL's layering. The adal-angular package contains two JS files: a core layer (adal.js) that does not depend on AngularJS and a top layer (adal-angular.js) that depends on AngularJS.
https://github.com/AzureAD/azure-activedirectory-library-for-js/tree/dev/lib
Apps can include either depending on the functionalities they need.

One could argue that ADAL should ship two NPM packages, but at the moment this is not the case. If the type package needs to match the NPM package, it needs to be a single package with two .d.ts files.

[ghost]

So how are these imported? Does one do import adal = require("adal-angular/lib/adal"); and import adalAngular = require("adal-angular/lib/adal-angular");?

[mmaitre314]

They are typically loaded via <script>:

<script src="scripts/adal.js"></script>
<script src="scripts/adal-angular.js"></script>

I got them to load as AMD once (in JS, haven't tried TS for that) but that required shims:

require.config({
  paths: {
    'adal': 'bower_components/adal-angular/dist/adal.min',
    'adal-angular': 'bower_components/adal-angular/dist/adal-angular.min',
    'angular': 'bower_components/angular/angular.min',
    'angular-route': 'bower_components/angular-route/angular-route.min',
  },
  shim: {
    'adal-angular': ['adal', 'angular'],
    'angular': {
      exports: 'angular'
    },
    'angular-route': ['angular']
  },
});

[eamodio]

Any chance this will get resolved? Not having the @types package match the npm package name basically makes the types package useless.

[craxal]

Typescript has a /// <reference types="..." /> directive now. Perhaps we can use this to reference the misnamed adal types package as a workaround. See https://www.typescriptlang.org/docs/handbook/triple-slash-directives.html.

[ghost]

PRs welcome.

[aciccarello]

As far as I can tell, the following changes are needed:

• ADAL definitions need to be published under @types/adal-angular to match npm package name
• adal-angular.d.ts needs to be included with index.d.ts (possibly under a "adal.angular" namespace?)
• adal-node definitions should be written under a separate package (see No Typescript typings AzureAD/azure-activedirectory-library-for-nodejs#142)
• Configuration properties should include jsdoc (see source for adal.js)
• Add callback: (errorDescription?: string, token?: string, error?: any) => void to config interface
• Add callback, popup, isAngular, REQUEST_TYPE, and CONSTANTS properties to authentication context interface

Any suggestions for PRs to fix the main issue of renaming the @types package and linking the adal-angular.d.ts file?

For reference, I am importing the library with the following imports:

import AuthenticationContext from 'adal-angular';
import 'adal-angular/lib/adal-angular';

// The AuthenticationContext needs to be globally available for adal-angular
window['AuthenticationContext'] = AuthenticationContext;

[amarzavery]

This PR AzureAD/azure-activedirectory-library-for-nodejs#176 adds type definitions for the node.js specific adal library (adal-node) along with proper tests in TS.

[amarzavery]

as of version 0.1.25 adal-node supports type definitions.

[husseinkorly]

I'm using adal-angular type definition, and when I try to import AuthenticationContext, it compiles fine but the browser keep giving me this error ERROR TypeError: adal_angular_1.default is not a constructor !!!
I'm importing like this:
import AuthenticationContext from 'adal-angular';

[aciccarello]

@husseinkorly That's not a type definition issue. It is actually a TypeScript compiler change. You'll need to do the following import.

import * as AuthenticationContext from 'adal-angular';

[husseinkorly]

@aciccarello this makes everything available under AuthenticationContext name space, but I still cannot access to the AuthenticationContext in the index.d.ts !
I'm trying to do something like this new AuthenticationContext(config)

[aciccarello]

I am able to create an instance of AuthenticationContext using that import.

    this.context = new AuthenticationContext(config);

Then to access the AuthenticationContext type I use the adal namespace.

  protected context: adal.AuthenticationContext;

What are you trying to do with new AuthenticationContext(config) in a d.ts file?

[husseinkorly]

@aciccarello Here's what I'm trying to do: I'm writing an auth service using adal-angular:
private _config: AuthenticationContextOptions;
private _context: AuthenticationContext ;
constructor(){ this._config = environment.adalConfig; this._context = new AuthenticationContext(this._config); }

[craxal]

I would think you'd want something like this:

// Import the AuthenticationContext constructor from the `adal-angular` module.
import { AuthenticationContext } from "adal-angular";
...
this.context = new AuthenticationContext(config);

or

// Import everything from the `adal-angular` module into a variable.
import * as adal from "adal-angular";
...
this.context = new adal.AuthenticationContext(config);

[husseinkorly]

@craxal do you mean importing from adal-angular or @types/adal-angular ?

[craxal]

@husseinkorly Either/or, it should work for both Typescript and your module loader. The key is knowing how the module exports what you need.

By looking at the @types/adal-angular package, the default export is the AuthenticationContext class (constructor), so you'll want to import like this:

import { AuthenticationContext } from "adal-angular";
...
this.context = new AuthenticationContext(config);

The module has other exports, so you'll want to import them as needed like this:

// If you want everything:
import * as adal from "adal-angular";

// ...or just specific things:
import { LoggingLevel } from "adal-angular";

To be sure, you can use a debugger. Drop a breakpoint before you first use the import and examine the import variable's value.

[aciccarello]

Okay, so I was using the old @types/adal types. I've tried to use the @types/adal-angular definition and run into some problems. The adal-angular project defines the AuthenticationContext as the export if used as a module or it defines a global variable.

https://github.com/AzureAD/azure-activedirectory-library-for-js/blob/c306c17e46c108ae680bd90829e8e98fb65416a1/lib/adal.js#L1930-L1935

This does not seem to work with the import * as AuthenticationContext syntax. Importing the AuthenticationContext as the default export throws a runtime error. I believe this is related to the --allowSyntheticDefaultImports flag.

Attempt 1

import * as AuthenticationContext from 'adal-angular';

const config: any = {};
const context = new AuthenticationContext(config);

TypeScript throws the following:

error TS2351: Cannot use 'new' with an expression whose type lacks a
call or construct signature.

Attempt 2

The adal-angular module does export an inject helper. However, this is not defined in the type definition.

import { inject, AuthenticationContextOptions } from 'adal-angular';

const config: AuthenticationContextOptions = {};
const context = inject(config);

error TS2305: Module '"C:/Users/username/Documents/project/node_modules/@types/adal-angular/index"' has no exported member 'inject'.

Proposed Solution

Update the type definition to export AuthenticationContext as the export and as a namespace.

// In module contexts the class constructor function is the exported object
export = AuthenticationContext;

// This class is defined globally in not in a module context
declare class AuthenticationContext {
  constructor(config: AuthenticationContext.Config) {}
  // ...
}

declare namespace AuthenticationContext {
  export interface Config {}
  // any other interfaces to export
}

Other changes

• redirectUrl should be optional in the config options
• add config to AuthenticationContext class
• export import helper
• define global Logging config object

I came up with an initial implementation these changes (following the module-class.d.ts template). They need to be improved and testing added before I can open a PR.

[unindented]

@aciccarello yeah, the problem is that adal-angular exports the AuthenticationContext with module.exports = AuthenticationContext, which can't be expressed with ES2015 modules.

Instead of your proposed solution, TypeScript 2.7 seems to be pushing users to use the --esModuleInterop flag:

Under the new --esModuleInterop these two issues should be addressed:

A namespace import (i.e. import * as foo from "foo") is now correctly flagged as uncallabale. Calling it will result in an error.
Default imports to CommonJS/AMD/UMD are now allowed (e.g. import fs from "fs"), and should work as expected.
Note: The new behavior is added under a flag to avoid unwarranted breaks to existing code bases. We highly recommend applying it both to new and existing projects. For existing projects, namespace imports (import * as express from "express"; express();) will need to be converted to default imports (import express from "express"; express();).

You can read more here: https://www.typescriptlang.org/docs/handbook/release-notes/typescript-2-7.html#support-for-import-d-from-cjs-form-commonjs-modules-with---esmoduleinterop

[aciccarello]

@unindented I hadn't seen that information so thanks for highlighting it.

I still wonder if the the type definition should be changed to match the module implementation. While using the flag may fix things, I don't think developers should be required to use it. I haven't tested it but I assume you should be able to still import the AuthenticationContext if the type definition was export = AuthenticationContext; and if the --esModuleInterop flag is enabled .

[amarzavery]

I think if adal-angular can add a default export in the source code over here.

if (typeof module !== 'undefined' && module.exports) {
        module.exports = AuthenticationContext;
        module.exports.default = AuthenticationContext; // This extra line would be helpful
        module.exports.inject = function (conf) {
            return new AuthenticationContext(conf);
        };
}

This should enable import AuthenticationContext from "adal-angular"

[aciccarello]

@amarzavery While I agree that they should also add it as a default export, I think the DefinitelyTyped repo should match the existing implementation.

According to the DefinitelyTyped README

For an NPM package, export = is accurate if node -p 'require("foo")' is the export, and export default is accurate if node -p 'require("foo").default' is the export.

I've created #24727 with this change and some missing properties.

[aciccarello]

The @types/adal-angular package has been updated. Types are now defined on the AuthenticationContext namespace (e.g. AuthenticationContext.Options).

The only other issue with the type definitions that I am aware of are the missing angular service types that were removed in 87ad3c1.

[orta]

Hi thread, we're moving DefinitelyTyped to use GitHub Discussions for conversations the @types modules in DefinitelyTyped.

To help with the transition, we're closing all issues which haven't had activity in the last 6 months, which includes this issue. If you think closing this issue is a mistake, please pop into the TypeScript Community Discord and mention the issue in the definitely-typed channel.