Jsdoc values as namespaces#20198
Conversation
1. In Javascript, support type references to class expressions whose
name is obtained via "static property assignment" to another class like
so:
```ts
function Outer() {
this.y = 2
}
Outer.Inner = class { }
/** @type {Outer.Inner} */
var inner;
```
2. In Javascript, support type references to properties of function and
class expressions that are assigned to a variable, like so:
```ts
var Outer = function {
this.y = 2
}
Outer.Inner = class { }
/** @type {Outer.Inner} */
var inner;
```
Now it turns out that we don't support Closure's property declaration
style, which doesn't require assignment in the constructor:
```js
class C {
constructor() {
/** @type {number} */
this.n
}
}
```
This means that we still miss basically all the properties of class.
Also the 'conflicting declarations' message still mentions absolute
paths :(
|
Note that chrome devtools doesn't improve that much because now we don't recognise devtools' way of declaring properties. We expect assignments, but it uses just a type annotation: class C {
constructor() {
/** @type {number} x */
this.x;
}
} |
Currently only applies to property accesses, but maybe should apply to everything.
This version isn't done yet and I think it still causes failures in the test suite.
…c-values-as-namespaces
|
After discussion with @weswigham and @mhegazy, I moved the bulk of the solution to the binder, which results in a more general solution. I still have some cleanup to do, and the current PR won't cover IIFEs or certain other patterns, but it's better than before. I also added recognition of type-annotation-as-declaration: /** @type {number} */
p.x;
class C {
constructor() {
/** @type {string} */
this.x
}
}This, too, needs some cleanup, since I originally only intended it to work for the latter case. |
This means that Javascript property assignments always create a namespace, never statics on a class. The ES5->ES6 class refactoring still needs to be updated.
Declaring values didn't work before.
export default fails right now; I haven't got it to work and it's not in dev tools, so I don't know if it's worth the effort.
Like: `var SDK = {}`.
These are normally generated by the Chrome dev tools build; adding them
gets rid of more than 10,000 errors
This removes tons of errors.
Don't issue a "multiple declarations must have the same type" error for JS static property assignments, because these don't appear to have a type in this case.
|
OK, I think this is in a good state now. Thanks @weswigham and @mhegazy for your help. Mind taking a look? |
weswigham
left a comment
There was a problem hiding this comment.
Some small comments, but it looks like a pretty good change!
|
|
||
| // Look up the function in the local scope, since prototype assignments should | ||
| /** | ||
| * For nodes like `x.y = z`, declare a member 'y' on 'x' if x was a function. |
There was a problem hiding this comment.
Is the if x was a function qualifier in this comment still true?
There was a problem hiding this comment.
Updated: It's now function, class or not declared. I have a change that will make it also {}, but that's in a branch that I'll make a PR for after this PR is in.
| function mergeSymbol(target: Symbol, source: Symbol) { | ||
| if (!(target.flags & getExcludedSymbolFlags(source.flags))) { | ||
| if (!(target.flags & getExcludedSymbolFlags(source.flags)) || | ||
| source.valueDeclaration && source.valueDeclaration.kind === SyntaxKind.Identifier || |
There was a problem hiding this comment.
I'd say that there should probably be an extra symbol flag you set on these symbols to imply this, rather than relying on the value declaration being an identifier implying this.
There was a problem hiding this comment.
Good idea, for now at least. There are still symbol flags to burn.
| } | ||
|
|
||
| function errorNextVariableOrPropertyDeclarationMustHaveSameType(firstDeclaration: Declaration, firstType: Type, nextDeclaration: Declaration, nextType: Type): void { | ||
| if (isIdentifier(firstDeclaration) || isIdentifier(nextDeclaration)) { |
There was a problem hiding this comment.
Again, I really feel like this should be a flag on the symbol and not be information implicitly derived from the declaration being an identifier.
And update some doc comments
| else { | ||
| isLegalPosition = propertyAccess.parent.parent.kind === SyntaxKind.SourceFile; | ||
| } | ||
| if (!isPrototypeProperty && (!targetSymbol || !(targetSymbol.flags & SymbolFlags.Namespace)) && isLegalPosition) { |
There was a problem hiding this comment.
we need to handle these recursivelly as well.. i.e. today we handel:
var outer;
outer = function() {};
outer.inner = function () {};but we should also handle:
var outer;
outer = function() {};
outer.inner = function () {};
outer.inner.another = function () {};There was a problem hiding this comment.
Good idea. I will do that in a follow-up PR. It will require touching even more of the existing code.
Fixes one blocking issue for compiling the chrome devtools.
name is obtained via "static property assignment" to another class like
so:
class expressions that are assigned to a variable, like so: