Skip to content

Translate namespaced type references in jsdoc #112

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

Closed
evmar opened this issue Mar 25, 2016 · 11 comments
Closed

Translate namespaced type references in jsdoc #112

evmar opened this issue Mar 25, 2016 · 11 comments

Comments

@evmar
Copy link
Contributor

evmar commented Mar 25, 2016

Input:

import {SomeClass} from './foo';

function bar(x: SomeClass) {}
bar(new SomeClass());

Current sickle output is something like:

var foo_1 = require('./foo');

/** @param {SomeClass} x */
function bar(x) {}
bar(new foo_1.SomeClass());

The problem is that SomeClass is known as foo_1.SomeClass in the output, so the type annotation in the comment is wrong. We need foo_1.SomeClass in the JSDoc.

One idea is to emit something like this in the TS:

SomeClass; // sickle:SomeClass

which will become (after TS compilation)

foo_1.SomeClass; // sickle:SomeClass

and then we could regex for these lines to build up the mapping between SomeClass and foo_1.SomeClass. Pretty crazy hacks though.

Related: #111

@evmar
Copy link
Contributor Author

evmar commented Mar 25, 2016

Causes warning:
WARNING - Bad type annotation. Unknown type SomeClass

@evmar
Copy link
Contributor Author

evmar commented Apr 16, 2016

We could probably find a way to hack around this with some terrible regexes, but it's make more sense to fix it in the compiler. So I filed this upstream:
microsoft/TypeScript#8120

@evmar
Copy link
Contributor Author

evmar commented Apr 25, 2016

I think a better fix for this is to undo the namespacing that TypeScript does.

E.g. the input is:

import {foo} from ...;
let x = foo;

TypeScript produces:

[module goop];
var x = bar_1.foo;

I'm suggesting we translate that back to:

[module goop]; var foo = bar_1.foo;
var x = foo;

To be honest, I'm not sure why TypeScript does the namespacing thing it does.

@evmar
Copy link
Contributor Author

evmar commented Apr 25, 2016

What's tricky about my proposal above is:

Input:

import {foo} from 'foo';
import * as bar from 'bar';

let x = foo;
let y = bar.a;

Current output:

var foo_1 = require('foo');
var bar = require('bar');
var x = foo_1.foo;
var y = bar.a;

I was suggesting identifying foo_1.foo as special and rewriting it, but somehow we'd need to know to leave bar.a alone.

@mprobst
Copy link
Contributor

mprobst commented May 6, 2016

Idea: if TSC choses a non-predictable prefix, can we just prime it on a prefix that we can predict?

import {Symbol} from 'module';
import * as prefix from 'module';

Symbol.access();
prefix.Symbol.access();

Gives:

"use strict";
var module_1 = require('module');
var prefix = require('module');
module_1.Symbol.access();
prefix.Symbol.access();

There's an API to find all code locations of a particular symbol:

languageService.getDocumentHighlights(fileName, positionOfSymbol, [fileName]);

So change imports to always be prefix imports, make up a prefix (_tsickle_import_123), and rewrite all locations where mentioned.

The second problem of TS dropping imports that are only used in type locations could maybe be detected with the same API?

@evmar
Copy link
Contributor Author

evmar commented May 6, 2016

(I thought more about the dropping imports problem and I think we're actually ok -- because in Closure-land there's a single namespace, types always show up in the value namespace. For any type Foo we import tsickle also must create a var Foo on that side for Closure, and then TS will preserve the import of Foo because of the value.)

Let me think about your renaming idea. You're saying we rewrite every import to be like import * as _tsickle_import_${origName} from ... so that we can find that name?

@mprobst
Copy link
Contributor

mprobst commented May 6, 2016

Yeah, but will the dropped import have the same name, i.e. will Closure understand which {Symbol} you're referring to in the @param annotation?

Re rewrite idea, yeah, that's it.

@evmar
Copy link
Contributor Author

evmar commented May 17, 2016

Upstream TS said they're not gonna help with this, so we need to do something. I will investigate your renaming idea. It looks promising.

evmar added a commit that referenced this issue May 26, 2016
Summary:
If you import a type from another module, when emitting the
type in Closure-land you also need to qualify it with its module
name.

This is the first step towards solving #112.
This is the easy but necessary part; further changes will add
the harder cases.

Reviewers: rkirov

Reviewed By: rkirov

Subscribers: typescript-eng

Differential Revision: https://reviews.angular.io/D125
evmar added a commit that referenced this issue May 26, 2016
Summary:
Extend the namespace-qualifying-types test to also test
an imported interface.

Fix one minor bug it revealed (the Closure-side "function Foo()"
generated from a "interface Foo" must also be exported if the
interface is exported.)

More work on #112.

Reviewers: rkirov

Reviewed By: rkirov

Differential Revision: https://reviews.angular.io/D126
@evmar
Copy link
Contributor Author

evmar commented May 27, 2016

It occurs to me that another fix for this would be to use Closure's ES6 imports mode.

@evmar
Copy link
Contributor Author

evmar commented May 27, 2016

evmar added a commit that referenced this issue Jun 2, 2016
Summary:
Translate
  import {foo} from 'bar';
as
  import {foo as tsickle_foo} from 'bar';
  const foo = tsickle_foo;

This is because JSDoc comments will use plain "foo", but the ES6->ES5
translation done by the TS emit will namespace all references to imports.

More work on #112.

Reviewers: rkirov, mprobst

Reviewed By: mprobst

Subscribers: mprobst, typescript-eng

Differential Revision: https://reviews.angular.io/D128
evmar added a commit that referenced this issue Jun 15, 2016
Summary:
Recall that if another module exports a type (e.g. an interface),
tsickle will create a Closure @record variable from it in the other
module.  When this module imports that type, for Closure purposes
we need to declare a reference to the *value* of that record variable.

But at TS compilation time that @record doesn't yet exist, so we must
forward declare it using "declare".  This change gives that forward
declaration a type.  By the time Closure gets to compilations using
this variable, this declaration has entirely disappeared; it's just
to make the TS compiler happy in the interim.

More work on #112.

Reviewers: rkirov

Reviewed By: rkirov

Subscribers: typescript-eng

Differential Revision: https://reviews.angular.io/D153
@evmar
Copy link
Contributor Author

evmar commented Jul 7, 2016

Fixed! The final fix was to make local name references, like so:

import {Foo} from 'bar';

becomes

import {Foo as tsickle_Foo} from 'bar';
const Foo = tsickle_Foo;  // if it's a value
type Foo = tsickle_Foo;  // if it's a type

@evmar evmar closed this as completed Jul 7, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants