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

Support exporting declaration of local class #12494

Closed
smhc opened this issue Nov 25, 2016 · 10 comments
Closed

Support exporting declaration of local class #12494

smhc opened this issue Nov 25, 2016 · 10 comments
Labels
Needs More Info The issue still hasn't been fully clarified

Comments

@smhc
Copy link

smhc commented Nov 25, 2016

TypeScript Version: 2.1.1 / nightly (2.2.0-dev.201xxxxx)

We have classes defined within a closure that are exposed externally. Closures are required due to angular dependency injection.

While it isn't possible to export the class for instantiation, it would be useful to at least be able to export the declaration of the class for compile time checking. e.g

namespace app {

   export function cameraFactory(lensService, buttonService)  {

       // Export the Camera/Photo class declaration somehow,
       // or merge with previous declaration?...
       // declare export Camera;
       // declare export Photo;

       class Photo {
           public view() {
               alert('photo');
           }
       }
       class Camera {
           public takePhoto() : Photo {
               lensService.focus();
               buttonService.press();
               return new Photo();
           }
       }

       class CameraFactory {
          public getCamera() {
             return new Camera();
          }
       }

      // Note that this function doesn't return a 'Camera', but there is a 
      // means to obtain one indirectly via other methods and third party frameworks.
       return new CameraFactory();
   }

   // Note the indirect creation of an internal 'Camera' object 
   // and the requirement to access local 'Camera' class declaration.
   // EDIT: modified code to better reflect what is happening - 
   // the Camera object is being obtained indirectly via a framework.
   let camera: Camera = angular.$injector.get('cameraFactory').getCamera();

   // let camera: Camera = AltCameraFactory.create();

   let photo = camera.takePhoto();
   photo.view();
}

We could declare the class separately in a d.ts but that seems redundant and error prone given the class is already being declared.

It could be re-written to do away with the factory and use the injected parameters on the Camera object itself rather than via closure. However there are much more complex factories out there that are dependent on access to closure variables, especially when also taking into account angular injection.

Perhaps it could narrow the type of export that occurs, such that you can simultaneously define the local class implementation and export the class declaration into the namespace for consumption:

namespace app {
  export function cameraFactory(lens) {
      export declaration class Photo {
        public view() {  alert('photo');   }
     }
  }
}

Or it could provide the body for the declaration of a prior empty declaration statement. e.g

namespace app {
  export declare class Photo;
  export function cameraFactory(lens) {
      class Photo declares app.Photo {
         public view() { ... }
      }
  }
}

Another alternative is access to the exported function as it's own namespace and export externally, e.g.

namespace app {
  export function cameraFactory(lens) {
      class Photo {
        public view() { ... }
      }
  }
  export type Photo = cameraFactory.Photo;
}

Or simply allow the exporting of a 'type' from functions:

namespace app {
  export function cameraFactory(lens) {
      class InternalPhoto {
        public view() { ... }
      }
      export type Photo = InternalPhoto;
  }
}

Or if you "export" a local class from a function, all that occurs is the type declaration is made visible at compile time and it cannot be instantiated.

Expected behavior:
An ability to export at least the declaration of the 'Camera' and 'Photo' class so it can be used for type checking and code completion externally.

Actual behavior:
"Modifiers cannot appear here" error when trying to export.
The inability to reference the 'Camera' or 'Photo' classes externally for compile time checking.

@aluanhaddad
Copy link
Contributor

aluanhaddad commented Nov 26, 2016

Note the indirect creation of an internal 'Camera' object
and the requirement to access local 'Camera' class declaration.

There is no such requirement. The compiler will infer the type

image

What we need is a way to capture the return type of the cameraFactory function. Since you say it is never called directly at any point this is tricky. Note that workaround I used above was

  const AltCameraFactory = cameraFactory(undefined, undefined);

We do not want this code to execute.
We can use the following runtime safe, but very hacky workaround

namespace app {
  // Hack
  type Factory = typeof cameraFactory;
  const cameraFactoryFactory = ((() => undefined as any) as Factory)(undefined, undefined);
  export type CameraFactoryFactory = typeof cameraFactoryFactory;
}

declare const AltCameraFactory: app.CameraFactoryFactory;

const camera = AltCameraFactory.getCamera();
const photo = camera.takePhoto();
photo.view();

See also #12342, #9889, and many other related issues.

@aluanhaddad
Copy link
Contributor

aluanhaddad commented Nov 26, 2016

Just to add a few thoughts, it is worth noting that letting TypeScript infer types wherever possible has many advantages. The incorrectly perceived need to get the type of Camera for annotating the local variable led to attempts to perform some very strange incantations and a request to add some very non-general syntax and a high degree of complexity to the language.

While the workaround above is a really awful hack and I would like to see this supported directly by the the language, it is worth noting that the current, highly useful behavior of return type inference for functions returning instances of local classes only makes sense given the structural nature of TypeScript's class types which in itself is not exactly accurate (that said, JavaScript classes are not really nominal either).

@smhc
Copy link
Author

smhc commented Nov 26, 2016

The incorrectly perceived need to get the type of Camera for annotating the local variable

This is due to a poor example rather than the lack of need. I should have used a better example to illustrate the scenario. The point is there is no way to infer the type through function calls, it is called by 'magic' using angular injection. The below example might illustrate it better:

export function cameraFactory($q, lens) {
    class Camera {
        public takePhoto() {
            lens.focus();
        }

        public turnOn() {
        }
    }
    return new Camera();
}
angular.module('CameraModule', [])
.factory('camera', cameraFactory)

// Angular injection, no user function is called. Angular just calls
// 'new' on the registered constructor and provides it as 
// a parameter at runtime using reflection.
function myOtherService(q$, camera: Camera) {
    camera.takePhoto();
}

request to add some very non-general syntax and a high degree of complexity to the language

I am by no means a language designer and would leave that to the experts, it's more to help illustrate the requirement. I'm hopeful people much more knowledgeable can recognise the requirement and design a much more elegant solution.

Thanks for your feedback, I'll look further into those other issues.

@aluanhaddad
Copy link
Contributor

aluanhaddad commented Nov 26, 2016

It would be ideal if there was support for getting the return type of a function. Since that is not supported, you can still use terribly hackish workaround mentioned.
camera-module.ts

import {module, IQService} from 'angular';

/******* HACK *********/
const hack = ((() => undefined as any) as typeof cameraFactory)(undefined, undefined);
export type Camera = typeof hack;

export function cameraFactory($q: IQService, lens) {
  class Camera {
    takePhoto() {
      lens.focus();
    }

    turnOn() {
    }
  }
  return new Camera();
}
module('CameraModule', []).factory('camera', cameraFactory);

and you can consume it transparently
somewhere-else.ts

import {IQService, IPromise} from 'angular';
import {Camera} from './camera-module';

function myOtherService($q: IQService, camera: Camera): IPromise<Camera> {
  const {resolve, reject, promise} = $q.defer<Camera>();
  
  camera.turnOn();

  setTimeout(() => {
    resolve(camera);
  }, 1000);

  return promise.finally(() => camera.takePhoto());
}

@aluanhaddad
Copy link
Contributor

@smhc I realize I came off as abrasive and I sincerely apologize. My remarks were not so much to do with this issue, but rather venting frustration from having to constantly read needlessly verbose code like

export class UserList {
  showGrid: boolean = true;
  maxRows: number = 10;
  gridOptions: GridOptions = new GridOptions();
}

When it is so much easier, more readable, and provides better tooling to let the compiler infer the types. I see that this is not really related and I apologize for ranting about something largely unrelated.

@mhegazy
Copy link
Contributor

mhegazy commented Nov 26, 2016

why not export the class directelly? why does it need to be in the closure?

export class Camera {
    public takePhoto() {
        lens.focus();
    }

    public turnOn() {
    }
}
export function cameraFactory($q, lens) {
    return new Camera();
}
angular.module('CameraModule', [])
    .factory('camera', cameraFactory)

or export only a type/interface of this class

class CameraImpl {
    public takePhoto() {
        lens.focus();
    }

    public turnOn() {
    }
}

export type Camera = CameraImpl;

export function cameraFactory($q, lens): Camera {
    return new Camera();
}

angular.module('CameraModule', [])
    .factory('camera', cameraFactory)

@smhc
Copy link
Author

smhc commented Nov 26, 2016

Thanks for both of those examples. I am indeed doing something like this in some cases. And using a angular 'service' rather than a 'factory' where possible which simplifies things even further. But that often requires binding all injected services (e.g lens) to 'this' via the constructor - or declaring them somehow.

In your example, is there a way to declare 'lens' in CameraImpl for use locally without declaring it on the global scope? We can pass it to the constructor and bind it to 'this' which is what I do in many cases. But it gets messy, and I have a case where it is impractical due to rebinding 'this' when called. If there is a way to declare the closure variables local to the class that might be a better solution.

That aside the example above is greatly simplified. In reality the factories are much more complex and return an inner class/object which in turn has methods for instantiating other inner classes. We also have cases where the internal class objects are later used as the prototype for the creation of a new object. So it would be really useful to have access to that declaration to help declare the type of those new objects.

The generic problem is we have classes internal to a function closure that are exposed indirectly. The key word being 'indirectly', which means we can't use type inference.

I'd be surprised if this wasn't a problem more people are encountering. I can only assume people are solving it by writing the declaration and using 'implements' on the internal class.

@smhc
Copy link
Author

smhc commented Nov 28, 2016

There is a similar issue posted to stack overflow:
http://stackoverflow.com/questions/31805817/typescript-angular-design-alternative-way-of-passing-dependencies
But with no solution other than not using closure variables.

My solution to this problem is to use static members to hold the injected dependencies. This results in the variables being available in the 'static' closure object. It's more verbose having to pass all the services and results in similar code. So I still think a way to either export inner class declaration or a way to declare closure variables is required.

The code looks similar to:

export class Camera {
    static lens;
    public static initialise(lens) {
      Camera.lens = lens;
    }
    public takePhoto() {
        Camera.lens.focus();
    }

    public turnOn() {
    }
}

function cameraFactory($q, lens): Camera {

    // Pass injected dependencies to static members rather 
    // than directly use the one from this closure
    Camera.initialise(lens);

    return new Camera();
}

angular.module('CameraModule', [])
    .factory('camera', cameraFactory)

I'm not sure if using static members is better or worse than assigning 'lens' to the Camera instance like the angular docs suggest:
https://angular.io/docs/ts/latest/cookbook/dependency-injection.html#!#di-inheritance

@mhegazy
Copy link
Contributor

mhegazy commented Nov 28, 2016

The generic problem is we have classes internal to a function closure that are exposed indirectly. The key word being 'indirectly', which means we can't use type inference.

That is the part i do not understand. the class can be put anywhere, you put it inside the function to hide it from the other declarations in the file, yet you want to expose it. so why not lift the declaration higher, and pass to its constructor any thing it captures today?

@mhegazy mhegazy added the Needs More Info The issue still hasn't been fully clarified label Nov 28, 2016
@smhc
Copy link
Author

smhc commented Nov 28, 2016

you put it inside the function to hide it

Not necessarily hide it, but to give access to injected dependency context without having to pass it around and assign it to every child object. People often exploit closures for the hiding/revealing pattern but their main purpose (debatable) is for providing 'context'. I give an example below.

why not lift the declaration higher, and pass to its constructor any thing it captures today?

As you can see in my previous comment this is what I've done, and is what the angular docs hint at doing. But you can make that argument for all closures in javascript - why not just pass parameters?.

You also don't always want to assign a service directly to the instance. The services are singletons and aren't modelled as members of the instance, they are modelled as services that are available as part of the context. It doesn't make a lot of sense to me for almost every object to have a member of 'this.$q'. It gets even more complicated with nested object instantiation. e.g.

class Camera {
   constructor(private $q, private $logger, private clickService) {
   }
   public click() {
      this.clickService.click();
   }
}

class DSLRCamera extends Camera {
   constructor($q, $logger, private lensService, clickService) {
     super($q, $logger, clickService)
   }
   public focus() {
       this.lensService.focus();
   }
}

function cameraFactory($q, $logger, lensService, clickService) {
   return new DSLRCamera($q, $logger, lensService, clickService)
}

compared to this code:

function cameraFactory($q, $logger, lensService, clickService) {
   class Camera {
     public click() {
        clickService.click();
      }
   }
   class DSLRCamera extends Camera {
       constructor() {
          super();
       }
       public focus() {
         lensService.focus();
       }
   }
   return new DSLRCamera()
}

For the top example the service dependencies are typed out five times vs just once for the second example. The lists in the first example also need to be in the right order and maintained every time it changes.

This list can grow quite large for deeply nested objects and requires updating the list for the entire chain of object constructors for each added dependency. It's at least one of the great things about using dependency injection and closures which typescript doesn't have a lot of support for imho.

Some of the benefits of using the closure-

  • Not cluttering 'this' on the instance (more useful if the keys are iterated)
  • Not needing to use 'this.' to use a service that really isn't part of the instance
  • Less verbose and not needing to pass large number of parameters in correct order
  • Inner classes are scoped to the function namespace (depending on a solution to the problem this may not always be the case)

I've overcome a few of the issues by using static members but it's still not a great solution in my opinion.

@mhegazy mhegazy closed this as completed Dec 29, 2016
@microsoft microsoft locked and limited conversation to collaborators Jun 19, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Needs More Info The issue still hasn't been fully clarified
Projects
None yet
Development

No branches or pull requests

3 participants