Skip to content
This repository has been archived by the owner on Feb 26, 2024. It is now read-only.

Emit classes with private constructors. #229

Closed
mprobst opened this issue Jan 29, 2016 · 7 comments
Closed

Emit classes with private constructors. #229

mprobst opened this issue Jan 29, 2016 · 7 comments

Comments

@mprobst
Copy link
Contributor

mprobst commented Jan 29, 2016

e.g.

function Foo() {
  /** @type {Foo.Bar_} */
  this.bar;
}

/** @private */
Foo.Bar_ = function() {};
@lambertjamesd
Copy link
Contributor

lambertjamesd commented Jun 29, 2016

goog.provide('test.PrivateConstructor');

/** 
 * @private
 * @constructor
 */
test.PrivateConstructor = function() {}

/**
 * @constructor
 */
test.PrivateConstructor.ChildPublicConstructor = function() {};

This will generate incorrectly too. The problem seems to be handling private constructors incorrectly. Right now clutz treats the class entire as private when its constructor is marked as private. It should only make only the constructor private.

Constructors marked @Private can only be instantiated by code in the same file and by their static and instance members.

https://developers.google.com/closure/compiler/docs/js-for-compiler#tags

Visibility on constructors will be released with TypeScript 2.0
microsoft/TypeScript#6885

So for now, the best solution would be just to ignore the private annotation on constructors

@lambertjamesd
Copy link
Contributor

lambertjamesd commented Jun 29, 2016

This commit fixes the issue
lucidsoftware@4384cff

EDIT: Just noticed this commit does not update any of the units tests, I can update the tests if this is agreed solution to the problem

@rkirov
Copy link
Contributor

rkirov commented Jun 29, 2016

I think the original bug report was different that the issue @lambertjamesd is bringing up. However, I cant repro the original issue, so lets continue on the private constructor issue (changing the issue title).

There are three distinct TypeScript concepts that are overloaded in the symbol test.PrivateConstructor - a type, a constructor for that type and a namespace where other object can live.

We agree that the constructor should be skipped (or emitted as private in future TS). It is definitely a bug that the namespace gets skipped because of the @Private annotation.

The type is the tricky one, where I think closure does allow you to goog.require it and use it in a type position, but our generation is greatly simplified by collapsing those instances to PrivateType and not having to emit anything. Do you have opinions on this one?

I think there is a way to fix the issue with the namespace, while still not emitting the constructor (nor type), by directly calling walkInnerSymbols but not walk in the case of private constructor. I would prefer that change because, it is more incremental and we do have a large number of internal users.

@lambertjamesd
Copy link
Contributor

A namespace wont cut it. The following works in closure

goog.provide('test.Foo');

/**
 * @private
 * @constructor
 */
test.Foo = function() {}

test.Foo.factory = function () {
  return new test.Foo();
}

test.Foo.prototype.method = function () {
  // Stuff
}

Some other file

goog.require('test.Foo')

test.Foo.factory().method()

Not only can it be a type, but you can access all of its members as well. The only thing marking the constructor as private will do is disallow calling new on it new test.Foo()

I think the right approach is to simply ignore the annotation for constructors until TypeScript 2.0.

@rkirov
Copy link
Contributor

rkirov commented Jun 30, 2016

That's a fair point, let's emit the constructor.

I think in that case PrivateType emitting code will need to be changed too, otherwise factory will have return type PrivateType which is not very usable from TS.

Can you add the factory static method to the private_types test in your PR?

@rkirov rkirov changed the title Fields referencing a private type emitted with type Emit classes with private constructors. Jun 30, 2016
@lambertjamesd
Copy link
Contributor

Here is the pull request
#300

@rkirov
Copy link
Contributor

rkirov commented Jul 11, 2016

merged #300

@rkirov rkirov closed this as completed Jul 11, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants