microsoft / TypeScript

TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
https://www.typescriptlang.org
Apache License 2.0
100.18k stars 12.38k forks source link

Proposal: The internal modifier for classes #5228

Open rbuckton opened 8 years ago

rbuckton commented 8 years ago

The internal modifier

Often there is a need to share information on types within a program or package that should not be accessed from outside of the program or package. While the public accessibility modifier allows types to share information, is insufficient for this case as consumers of the package have access to the information. While the private accessibility modifier prevents consumers of the package from accessing information from the type, it is insufficient for this case as types within the package also cannot access the information. To satisfy this case, we propose the addition of the internal modifier to class members.

Goals

This proposal aims to describe the static semantics of the internal modifier as it applies to members of a class (methods, accessors, and properties).

Non-goals

This proposal does not cover any other use of the internal modifier on other declarations.

Static Semantics

Visibility

Within a non-declaration file, a class member marked internal is treated as if it had public visibility for any property access:

source.ts:

class C {
    internal x: number;
}

let c = new C();
c.x = 1; // ok

When consuming a class from a declaration file, a class member marked internal is treated as if it had private visibility for any property access:

declaration.d.ts

class C {
    internal x: number;
}

source.ts

/// <reference path="declaration.d.ts" />
let c = new C();
c.x = 1; // error

Assignability

When checking assignability of types within a non-declaration file, a class member marked internal is treated as if it had public visibility:

source.ts:

class C {
    internal x: number;
}

interface X {
    x: number;
}

let x: X = new C(); // ok

If one of the types is imported or referenced from a declaration file, but the other is defined inside of a non-declaration file, a class member marked internal is treated as if it had private visibility:

declaration.d.ts

class C {
    internal x: number;
}

source.ts

/// <reference path="declaration.d.ts" />
interface X {
}

let x: X = new C(); // error

It is important to allow assignability between super- and subclasses from a declaration file with overridden members marked internal. When both types are imported or referenced from a declaration file, a class member marked internal is treated as if it had protected visibility:

declaration.d.ts

class C {
    internal x(): void;
}

class D extends C {
    internal x(): void;
}

source.ts

/// <reference path="declaration.d.ts" />
let c: C = new D(); // ok

However, this does not carry over to subclasses that are defined in a non-declaration file:

declaration.d.ts

class C {
    internal x(): void;
}

source.ts

/// <reference path="declaration.d.ts" />
class D extends C {
    internal x(): void; // error
}
kitsonk commented 8 years ago

Would the following also error:

declaration.d.ts

class C {
    internal x(): void {};
}

source.ts

/// <reference path="declaration.d.ts" />
class D extends C {
    x(): void {}; // error?
}

How does this differ from the more common language semantic protected?

The non-goal is not to address this in other declarations, but couldn't this start to becoming confusing for someone trying to utilise a package or library? For example, I would assume the following you would want to work this way:

declaration.d.ts

interface InterfaceC {
}

class C implements InterfaceC {
    internal x(): void {};
}

source.ts

/// <reference path="declaration.d.ts" />
class D implements InterfaceC {
}

const c: C = new D(); // error, but why? I implemented the interface properly.

When targeting ES6, do you see any changes, considerations in the emit?

Do you have a more practical use case of where this pattern would resolve significant problems? I can understand how for a consumer it could clean up an API, but at the same time, it could be really confusing where you are "hiding" things that will always exist at runtime from compile time. It almost seems like a six of one, half a dozen of another.

jbondc commented 8 years ago

An alternative to modifiers is:

package.d.ts

/// <reference path="declaration.d.ts" />
internal {
   C.x;
}

An internal {} block marks which parts of the public api are hidden.

sccolbert commented 8 years ago

+1 to something like this

alisabzevari commented 8 years ago

+1 for this.

ToddThomson commented 8 years ago

Internal modifier for classes and their properties/methods within a component/program would allow greater scope for Typescript identifier shortening. +1 for this proposal.

shmuelie commented 8 years ago

I think I'd want to extend internal to also work on classes.

It would work pretty much like the --stripInternal compiler flag works now, except without the need to add JSDoc to the class.

Additionally, though this may need some exploring, it could be used to keep classes within a module. So for example:

//file1.ts
module A.B
{
    internal class C { }

    var c: C = new C(); //Works
}

//file2.ts
module A
{
     var d: A.B.C = new A.B.C(); //Error because C is only available inside A.B module scope.
}

//file3.ts
module A.B
{
    var e: A.B.C = new A.B.C(); //Works, back in scope
}
tetsuharuohzeki commented 8 years ago

I'm polishing classes' members' openness of RxJS like https://github.com/ReactiveX/RxJS/pull/1244, then I wanted this feature to control openness in a complexed class dependencies.

JonathanMEdwards commented 8 years ago

+1 Having to write @internal all over the docs is currently my biggest pain point with TS. Which is another way of saying that things are pretty good - good job guys :)

TheLevenCreations commented 8 years ago

+1 any progress on this?

bartzy commented 8 years ago

+1, this would be awesome to have.

jelbourn commented 8 years ago

This would be really nice for Angular components where you need properties/methods to be public to be accessed by the template, but want them to be private to other users that are consuming your components. The current /** @internal */ feature is problematic because it causes types to fail to satisfy implemented interfaces when the d.ts is compiled downstream.

kito99 commented 8 years ago

+1 This would really help with testing; right now you have to make methods public that you really just want to be visible for testing but not part of the public API.

davidstellini commented 8 years ago

Agreed. This would make sense for services, where you can have internal methods that can only be accessed from other services if necessary, but not the API consumer.

jmcota commented 8 years ago

+1 internal modifier would be a big Christmas gift.

domchen commented 8 years ago

We really need this feature too. I am a developer of a HTML5 engine called Egret. Our engine contains plenty of internal APIs which are not allowed to accessed by user, otherwise it may cause internal problems. Currently, we have to make them as public and add some documents for it says ' do not use this method! '. It is really hard for us to design a safe and stable engine API because of the lacking of internal . Hope that you could add this feature soon, thanks in advance :)

zpdDG4gta8XKpMCd commented 8 years ago

excuse me, i know what a module or a namespace is, but what is a package?

zpdDG4gta8XKpMCd commented 8 years ago

related https://github.com/Microsoft/TypeScript/issues/321

kitsonk commented 8 years ago

excuse me, i know what a module or a namespace is, but what is a package?

CommonJS Packages which npm and other JavaScript package managers build upon.

zpdDG4gta8XKpMCd commented 8 years ago

collection of modules, assets etc

looks like something that only makes sense in NodeJS realm

a rigid definition would be a plus

kitsonk commented 8 years ago

looks like something that only makes sense in NodeJS realm

No, AMD modules are organised into packages, although the AMD loader doesn't read the package.json. Bower deals with packages and specifically is targeted at a browser context. It is a pretty well understood concept of a set of modules and other resources (e.g. CSS) that serve a well defined purpose.

a rigid definition would be a plus

The Internet has (sometimes) thrived because of non-rigid definitions.

zpdDG4gta8XKpMCd commented 8 years ago

you might be right about the Internet in general, but this is a

image

which is a place where rigid definitions are more than welcome

andrewalderson commented 7 years ago

+1 for this. I am currently writing a library that uses builders to construct instances of classes. I have the builder and its target in the same module. The target class should have an internal (or private) constructor because it should only be constructed by the builder. Currently it can't unless I use Reflect to construct the target class. Having a module internal modifier would fix this issue. Would really like to see this in TypeScript.

axefrog commented 7 years ago

This is an incredibly useful proposal and, with great respect, I'm a bit surprised it doesn't seem to be getting any love from the dev team, given the number of comments and upvotes.

I need various properties exposed for manipulation by publicly-exported functions that take an instance of a given class and the perform manipulation on members that public to the package, but private to external consumers. The only way to do this currently involves hacks:

The example below is contrived; I'm actually using classes only as minimalistic containers for structural data, but providing a functional interface for manipulation thereof.

export interface MyType {}

export class MyTypeImpl implements MyType {
  constructor(public _k: number) {};
}

// External consumer has no access to the 
export function Foo(x: MyType): MyType;
export function Foo(x: MyTypeImpl): MyTypeImpl {
  return new MyTypeImpl(x._k + 1);
}

Alternatively, I could do this:

export function Foo(x: MyType): MyType {
  const y = <MyTypeImpl>x; // but now I'm compiling this pointless variable into my JS output
  return new MyTypeImpl(y._k + 1);
}

Having to use an interface to hide internal members from consumers creates a whole bunch of pointless code throughout my codebase in order to see class internals.

mike-lischke commented 7 years ago

bopping this issue to keep it on the radar, it's important...

axefrog commented 7 years ago

Workaround for library authors is to use /** @internal */ on relevant class members in order to stop them from showing up in type definition files.

Soul-Master commented 7 years ago

+1 for this proposal. It should simplify logic for patching object that normally use in JavaScript library.

For current TypeScript, I use the following code to add internal property to external object.

// This signaure should be defined by other module.
interface Bar {
    publicProperty: string;
}

// Signature for using in this module
interface InternalBar extends Bar {
    _internalProperty?: string;
}

// Public API
type FooSignature = (obj: Bar, times: number) => void;

// Real Function
const Foo:FooSignature = function (obj: InternalBar, times) {
    obj._internalProperty = '1234';
    obj.publicProperty = 'test';
    times++;
};

export { Foo };

https://gist.github.com/Soul-Master/faf2e1d59c08bedf25d6fcf2722deff0

vultix commented 6 years ago

Is there any news on this? It would be a very much welcomed improvement.

ghost commented 6 years ago

Personally I'm more partial to friend access as you don't have to build multiple times to make use of the feature. It sounds like making something internal to a part of your project would require giving it its own build step.

mark-buer commented 6 years ago

If your needs are small, the following example might suffice. It exposes protected properties and methods via a subclass. It has many drawbacks and limitations, but it also has the important pro of avoiding any casts. Proper support for this feature (or something similar) can't come soon enough!

// For clarity, we define all the "internal" methods and properties here
abstract class InternalWidget {
    protected abstract set internalProperty(value: number);
    protected abstract internalOperation(): void;
}

class Widget extends InternalWidget {
    protected set internalProperty(value: number) {
        console.log("you have the privilege to set this");
    }
    protected internalOperation(): void {
        console.log("you have the privilege to invoke this");
    }
}

// The horrible hack and nasty boilerplate
abstract class InternalWidgetAccessor extends InternalWidget {
    static setInternalProperty(widget: InternalWidgetAccessor, value: number): void {
        widget.internalProperty = value;
    }
    static internalOperation(widget: InternalWidgetAccessor): void {
        widget.internalOperation();
    }
}

// Without
{
    const widget = new Widget();
    widget.internalProperty = 42;   // compiler error
    widget.internalOperation();     // compiler error
}

// With
{
    const widget = new Widget();
    InternalWidgetAccessor.setInternalProperty(widget, 42);  // no error
    InternalWidgetAccessor.internalOperation(widget);        // no error
}

Hope this helps someone out.

hmil commented 6 years ago

I'm not sure if introducing such a complex construct is worth the effort given that one can already achieve the same result with existing syntax. The only trade off is to export factories instead of classes (shouldn't you already be doing this anyway?), and then you get a beautiful solution:

export interface ITheClass {
  doStuff(): void;
}

export class TheClass implements ITheClass {

  // === Public API ===

  public doStuff() {
     this.doInternalStuff();
  }

  // === Visible for testing ===

  public doInternalStuff() {

  }
}

Your main export file then looks like this:

// This file is only exporting the public API and a factory method.
// Not only is this solving the problem the `internal` keyword is trying to solve,
// it also ensures I'm hiding implementation and instead only exposing interfaces.

export ITheClass from '...';

export function makeTheClass(): ITheClass {
  return new TheClass();
}

The test code can just new TheClass and get access to the test-only methods, but a user of the package does't see the symbol 'TheClass' and hence can't directly instantiate it.

I'm interested if someone can come up with a use case where the internal keyword would be preferable over the above pattern.

mark-buer commented 6 years ago

@hmil My main criticisms of your outlined approach are yet-more-boilerplate and inflexibility. In your chosen example, TheClass conveniently has zero constructor arguments. The pattern you are advocating for becomes onerous in code with constructors that (necessarily) take many arguments (and not dictionaries). Additionally, access to the class type is sometimes a requirement. Examples include: use of a framework like Angular where the classes need to be registered for injection, performing instanceof testing, and subclassing.

hmil commented 6 years ago

A factory function can always accept the same arguments as the constructor, which admittedly isn't going to be very DRY. When you need access to the class itself (for instanceof and such), you can still use the above pattern, just adapt the wording a little. Embed the factory function as a static function of the class and declare the constructor private. You'll need an extra interface to work with this solution.

Here's what you'd get:

export interface ITheClass {
  doStuff(): void;
}

export interface ITheClassTestingInterface extends ITheClass {
  doInternalStuff(): void;
}

export class TheClass implements ITheClass {

  public static create(...args): ITheClass {
    return new TheClass(...args);
  }

  public static createForTesting(...args): ITheClassTestingInterface {
    return new TheClass(...args);
  }

  private constructor(...args) { /* ... */ }

   // === Public API ===

  public doStuff() {
     this.doInternalStuff();
  }

  // === Visible for testing ===

  public doInternalStuff() {

  }
}

This may seem verbose but I would argue that it is worth the extra effort. My point stands: The tools currently available are powerful enough that you can achieve the result but also restrictive enough to enforce good practice (hide your implementations!).

I fear that introducing the internal modifier would just add useless confusion and introduce yet another way for careless developers to screw their packaging (think leaky encapsulation). The exact semantics of the keyword are hard to define (what is a JavaScript "package" anyway?) and maybe harder to understand. The question is: is the above workaround such a hassle that we want a dedicated keyword just for this manageable use case?

mark-buer commented 6 years ago

@hmil Here is a typical constructor from my code base (there are bigger ones than this):

class TheClass {
    constructor(
        private _project: Project,
        private _contextualState: ContextualStateMutator,
        private _flatNavigableTocItems: FlatNavigableTOCItems,
        private _highlightedTokenPosition: HighlightedTokenPosition,
        private _moveSelectedRegions: MoveSelectedRegions,
        private _resizeSelectedRegions: ResizeSelectedRegions,
        private _revealFlatNavigableTocItem: RevealFlatNavigableTOCItemActor,
        private _revealTokenLocality: RevealTokenLocalityActor,
        private _selectTokenLocality: SelectTokenLocality,
        private _selectTokenSpan: SelectTokenSpan,
        private _tokenLabelsMutator: TokenLabelsMutator,
        private _visibleFlatNavigableTocItemMutator: VisibleFlatNavigableTOCItemMutator,
        private _visibleTokenIndexMutator: VisibleTokenIndexMutator,
        private _selectedRegionModels: SelectedRegions,
        private _selectedTokenSpan: SelectedTokenSpan,
    ) { }
}

Implementing your suggested pattern on this class would be insanely verbose, and nicely demonstrates why a new keyword/language feature is so important.

hmil commented 6 years ago

I see how a factory method becomes a problem in your case :smile: I'm still not convinced there is no other way than the internal keyword though. Consider the following:

The example you gave suggests you are using dependency injection to create TheClass since that is a pretty large amount of arguments for a class you'd construct manually. In general DI helps with separating implementations from abstractions (that's basically all it does) so it should actually make a case against the internal keyword and not for it. In Java with Guice you would never ever invoke the constructor. Instead there would be something like this somewhere in your code:

bind(ITheClass.class).to(TheClass.class);

Of course we can't do exactly this in TypeScript because interfaces don't materialize in JavaScript. However the DI library you are using has to have a similar way to provide bindings. Which means you could keep the class constructor private and suggest the usage of the interface type rather than the class type.

Although in such situation the user will have access to the "private" API on the class object and will have to remember to only use the interface types from your module. Is it something you are trying to avoid?

mark-buer commented 6 years ago

@hmil DI isn't of direct concern to my problem at hand, although I am using it elsewhere for the UI.

I have a bunch of model classes that need to interact with each other and with an undo/redo mechanism, as well as with a persistence layer (dirty checking, generating diffs etc). Most importantly, this model is consumed by the rest of the application. Thus, I would like the "behind the scenes" interaction to be obviously marked as such, to be typesafe, and be not too verbose. I would also like the "behind the scenes" interaction to not be plastered all over the public interface of the model.

The consumer of the model (the rest of the app) needs to be using class types. Why?

Basically, I want something akin to Java's package access scope. I don't really care what "package access scope" means in the context of typescript, I'll make it work with whatever we (hopefully) end up with. If it was limited to the current compilation unit (file?) that'd be fine with me. The friend keyword would also do the trick.

Its hard to express the difficultly of working around this language limitation if you've never naturally run against it. Yes, everything can be worked around if you're willing to triple the amount of code you will write and maintain, or if you're willing to have a few points of unsafe casting here or there. Personally, the package access scope feature from Java is sorely missed.

hmil commented 6 years ago

@mark-buer Point made. Thanks for taking the time to explain it in such details.

robertorfischer commented 6 years ago

+1 for the Angular case. Component interfaces are terrible right now and very easy to misuse, especially in testing. An internal modifier would greatly improve things.

stillery-sw commented 6 years ago

+1

matthewmueller commented 6 years ago

I'd like to see this too, but have you guys considered the way Golang handles this? I find their approach quite elegant.

They let you have a directory called /internal, which only allows access to internal modules in the folder that contains that internal folder (and access from within that internal folder)

Graphically it looks like this:

/project
  /internal
    /component
      component.go // internal
  /api
    api.go // access to internal component
  main.go // access to internal component

/another-project
  main.go // no access to project's internal component
mike-lischke commented 6 years ago

@matthewmueller Too easy to mess up. An explicit keyword is much better. You don't need to look where the file is located to understand it's functionality.

westonpace commented 6 years ago

@matthewmueller That approach is limited to making an entire module internal or external. Usually I just want to mark a few members internal. For example, I have a tree-like structure that has node instances. The nodes need to be able to muck around with the tree in order to fulfill some of their operations. Part of the tree's API is internal (to allow the node's pseudo-private access) and part of the tree's API is public.

EisenbergEffect commented 6 years ago

@rbuckton Is there any status on this? It's been 2.5 years since this was proposed. I'm working on Aurelia vNext right now, completely written in TS. We could really make use of this feature...

DanielRosenwasser commented 6 years ago

As we work on project references (#3469), I think we have even more reason to hold off on this. For example, the current proposal relies on a distinction between a declaration in a .ts file and a .d.ts file, but project references heavily leverage .d.ts files.

be5invis commented 5 years ago

But is this issue all about preventing definitions of one interface being exported? We can add a export protected statement that exports one type but disallow accessing its internals with user code (thought the internals are still used by type checking).

protected export interface Engine { a: number, b: string }

A protected exported interface (or class' type half) would only outside to see its name and generic parameters (a.k.a. "kind" in FP). It prevents...

fatcerberus commented 5 years ago

Going to throw my proverbial hat in the ring here, I love internal in C# and would love to see it in TS. “Pure” encapsulation is nice, but fully decoupling the internals of a program/library is not always practical and then I end up just declaring stuff public when I really would prefer not to.

aleclarson commented 5 years ago

I've found a really neat trick. Not sure if many people know about it or not:

export class Foo {
  ['foo'](arg: number) {}
}

The foo method is hidden from IntelliSense, yet TypeScript still validates any callsites, so you can call foo from the internals without sacrificing type-safety. 🎉

I would recommend prefixing these "hidden methods" with an underscore, so users aren't tempted to call them if they happen upon them while looking through the source code.

edit: It's worth noting that foo is suggested in auto-complete when bracket notation is used on the instance of Foo.

ghost commented 5 years ago

@aleclarson I'm not sure I would recommend abusing what appears to be a bug in the tooling for this purpose...

aleclarson commented 5 years ago

@errorx666 I don't think it's a bug. Bracket notation with a string literal allows unsafe property access on objects, so this seems like a natural extension of that behavior. Obviously, we need confirmation from the TS team to know for sure.

fatcerberus commented 5 years ago

It doesn’t allow unsafe access though - unless the object’s type includes an index signature. Otherwise you can only access properties that are part of the type, same as with dot notation.

aleclarson commented 5 years ago

@fatcerberus That's true if you're using noImplicitAny (which you should be)