Repository navigation
In JS, assignment to an undeclared instance property should act as its declaration #22896
Description
Activity
Note that this is about checking
jsfiles, nottsguess where one would declare class properties anywaysI think it's a correct error, because it's an instance variable.
class Foo { constructor () { this.f = undefined } }should be enough to declarefas an instance variable; however, its type may end up beingundefinedinstead ofanyor some other appropriate type. It'd be better to annotate thethis.fassignment with@type.Reacted by Vitali Kuzmich@Kovensky Not sure what design goals of Salsa is but
this.f = undefinedin constructor seems unlike usual JavaScript.Reacted by Marvin HagemeisterTo be honest, I think adding properties to instances after the fact isn't something we can do under our current design.
Reacted by Mohsen AzimiWhat's being proposed? Would a set of a property on a class instance never be an error? That seems like going too far.
At least in Webpack they are fine with initiating the member with undefined in constructor.
See this comment: webpack/webpack#6869 (comment)
I think a solution here is to support
/** @property */and/or/** @member */tags on classes. this way the code does not need to be changed with athis.prop = undefined, just a comment added along with a type.similar to #15715.
Reacted by Mohsen Azimi- changed the title
[-]Salsa enforce class member declaration [/-][+]In JS, assignment to an undeclared instance property should act as its declaration[/+]on Mar 28, 2018 A
@propertyjsdoc may be the right solution for some code bases, but I don't think webpack is an example of one. For webpack to compile with checkJs, you have to add (at least) one line of code to fix the "Property does not exist" error. The question is which line you want to write:class C { constructor() { this.upfront = 1 // here?: this.late = undefined } } var c = new C(); // here?: /** @property */ c.late = 2;
Of the two,
this.late = undefinedis clearly better since it improves the definition ofCboth at edit time and compile time by making it easier for the compiler and the VM, respectively, to understand.The only reason I can think of to prefer
/** @property */in this case is if you want to check a code base without changing anything except comments. But in that case I think you are better off inserting// @ts-ignore FIXMEeverywherelateis referenced, the way Google does with errors it wants to ignore. For most projects, the whole point of introducing Typescript is to make changes based on the errors it produces. If some project authors aren't on board, then they won't be happy with any changes that Typescript precipitates — some are guaranteed to be non-idiomatic Javascript.One place where
/** @property */may make sense is an initializer that is not part of a class (or constructor function):function C(full = true) { this.basics = 'just the basics' if (full) initC(this) } function initC(c) { c.other = 'stuff goes here' c.db = somedb.connect() c.launchMissiles = function() { /* CLASSIFIED but definitely IRREVOCABLE */ } } C.prototype.afterInitialisation = function() { // refer multiple times to c.other, c.db, c.launchMissiles }
Since
initCinitialises a lot of properties, and presumably does so during construction much of the time, it wouldn't provide a runtime advantage to have a bunch of assignments likethis.other = undefinedin the constructor function, and the duplication would stand out in the source. Note that to get this to work, we'd need to type the parametercas well./** @param {C} c */ function initC(c) { /** @property */ c.other = 'stuff goes here' /** @property */ c.db = somedb.connect() /** @property */ c.launchMissiles = function() { /* CLASSIFIED but definitely IRREVOCABLE */ } }
Even in this scenario, the duplication in the constructor might be just as good as the additional doc comments in
initC.My vote is to look for some other motivating example than webpack, since it looks like the webpack changes just required one or two additions per class. Plus, those additions are good for the runtime performance of webpack.
Reacted by Vitali KuzmichI was thinking of adding it on the class/function declaration, e.g.:
/** * @property {number} a */ class C { m() { this.a; } }
or
/** * @class * @property {number} a */ function F() { this.m = function m () { this.a; } }
Reacted by Tomislav Jovanovic, Brian Bugh and Yaroslav Admin
TypeScript Version: 2.7.0-dev.201xxxxx
Search Terms:
Salsa, Class member, instance
TypeScript Version
2.7.2
Code
Expected behavior:
No error
Actual behavior: