phil294 / coffeesense

IntelliSense for CoffeeScript. LSP implementation / VSCode extension
MIT License
48 stars 9 forks source link

Positioning of JSDoc comments #1

Closed phil294 closed 1 year ago

phil294 commented 3 years ago

As commented here, block comments are often not where you'd expect them. Please read the specific issue for details. However, it refers to the official CoffeeScript compiler output.

In CoffeeSense, the input is somewhat modified, which is why in the linked case, the following would be enough for in-IDE type hints:

#
###*
# @param a {string}
###
method1 = (a) ->

#
###*
# @param b {string}
###
method2 = (b) ->

Note the extra # on top of the blocks. In many cases, you can also write ``###* ... ### which is even shorter but fails compilation inside objects

This is treated by tsserver as


/**
 * @param a {string}
 */

let method1 = function(a) {};

/**
 * @param b {string}
 */
let method2 = function(b) {};

Maybe there are better ways of achieving proper block comment positioning? Or even better, something like the extra # should be inserted automatically so the user does not have to care about it anymore. This is somewhat annoying as it requires rewriting source maps (similar to how variable declaration rewriting already works), but it's possible.

I want more feedback from smart people on this, so until then, this stays as is

robert-boulanger commented 2 years ago

There is one more issue when dealing with classes:

even when writing (note the fat arrow):

#
###*
# @param {string} b
###
method2 :  (b) =>
  console.log b

the output js results in

constructor: ->
  /**
  * @param {string} b
  */
  this.method2 = this.method2.bind(this);
  ...

within the constructor, and within the class body:

method2(b){
  console.log(b)
}

without any doc. So b is still treated as any ;(

phil294 commented 2 years ago

@robert-boulanger nice catch! but is there any real reason to use fat arrows for class method definition?

robert-boulanger commented 2 years ago

A real world scenario would be (and is) if an instance of such a class is for example, pushed into an reactive array in Vue3. As soon as a methodof this instance will be accessed through the Vue component the reactive array is.a member of, Vue creates a proxy out of our instance which causes that this gets lost unless the method is bound to the instance. Means we need a fat arrow.

phil294 commented 2 years ago

A real world scenario would be (and is) if an instance of such a class is for example, pushed into an reactive array in Vue3. As soon as a methodof this instance will be accessed through the Vue component the reactive array is.a member of, Vue creates a proxy out of our instance which causes that this gets lost unless the method is bound to the instance. Means we need a fat arrow.

I tried to reproduce what you just described: Vue 3 SFC Playground example

but I cannot see the class instance's this getting lost (method() can still access the class prop). Can you update this snippet to demonstrate the issue?

Another Vue-related issue I could think of would be a Vue 2 (old-school options api style) component, in which the Vue this gets lost inside the method:

Vue 2 SFC Playground example

here, this.vueProp inside Item::method is not reachable because the class instance's this takes precedence. But you could easily fix this by saving this to a var beforehand, as you surely know anyway. Having a class method invoke side effects is a bad idea anyway, in the latter example, method() should really be part of Vue methods:, not the class

In any case, I don't think the JSDoc comment block positioning issue for fat arrow methods can be reasonably fixed in extension code :-/ But it should be with the above linked CoffeeScript issue. I'll add a comment there

robert-boulanger commented 2 years ago

@phil294: It is indeed an options api but already Vue3. I'm porting currently a huge Quasar App. So the reactive array I mentioned, was not an array created with reactive(array), it just was meant as an array in a (reactive) Vue environment.

STRd6 commented 2 years ago

Since the positioning of block comments was why this revert happened https://github.com/phil294/coffeesense/commit/0a147d137dae1abeb06f512d4c0de804716df8e2 and it seems block comments are incorrectly positioned anyway... would it make sense to restore this?

It seems like it would fix a lot of my TypeScript issues and the 'empty comment' hack still works with it.

phil294 commented 2 years ago

@STRd6 edemaine's branch, which was in use for a short period before being reverted again as you noticed, while fixing non-block-commented lines, it still translates the following

#
###* @type {abc} ###
a = 1
a = '1'
b = 2

into

/** @type {abc} */
var a, b;
a = 1;
a = '1';
b = 2;

which is of course bananas, so it can't be used (yet) and the lsp still uses the baked in workaround which translates into

/** @type {abc} */
let a = 1;
a = '1';
let b = 2;

as expected. (debug with VSCode command CoffeeSense: Show generated JavaScript for current file)

So yes, the comment blocks are always broken without the leading empty comment hack #, but with it, only the latter version is correct.

Hope I understood you correctly.

I guess both logics could be combined too, if it's worth the effort. I'd be interested in seeing your concrete TS issues for that that you are seeing because of the current state of the extension, if you are willing to share?

STRd6 commented 2 years ago

I see what you mean...

It looks like without block comments edemaine's branch works great with destructuring but doesn't work with block comments.

{sep} = require "path"

->
  sep
// Current implementation
var sep

({sep} = require("path"));

(function() {
  return sep; // Later reference inside of a function causes TypeScript to be unable to infer type.
});
// edemaine's branch
var {sep} = require("path"); // TypeScript can figure this out automatically

(function() {
  return sep;
});

I tried compiling with the block comments in various places and found that destructuring and block comments really don't get along either.

For the example you gave here is what I got when running edemaine's branch:

/** @type {abc} */
var a;

a = 1;

a = '1';

var b = 2;

It seems almost reasonable but splitting the assignment onto a separate line isn't great.

I'll try and get a local dev version of CoffeeSense running with the CoffeeScript branch and see what happens.

edemaine commented 2 years ago

Time to work on my branch again I guess... Sorry, this fell off my radar for a while and it's taking a while to get back to.

phil294 commented 2 years ago

I guess both logics could be combined too, if it's worth the effort

I now did that and it seems to work fine. Idk why I didn't do this from the start. @STRd6 your {sep} example should now work fine with v1.10.0, see also this test. This gets transformed by ede branch already to var { abc } ... even though there is a block comment before it. Not without the something_else = 1 part though, but in that case, the extension transforms it. So I don't exactly know why, but everything works now (?)

.. as long as you do the # prefixing nonsense of course

STRd6 commented 2 years ago

@phil294 I've been trying out v1.10.0 and it's very good so far! I think most of the remaining issues have to be fixed in coffeescript's comment handling.

Thank you for your work!

STRd6 commented 2 years ago

Adding some links to CoffeeScript issues for reference:

https://github.com/jashkenas/coffeescript/issues/5417 https://github.com/jashkenas/coffeescript/issues/5415

Someday I may build up the courage to tackle these in a CoffeeScript PR.

surjikal commented 2 years ago

That alternative syntax from the tests is nice, thank you!

``###* ... ###

I'm wondering if there's value in adding a vscode command to insert a coffeesense / coffescript jsdoc comment stub. I keep forgetting the correct syntax and always come back to this github issue to remember.


EDIT: Just realized that ###* ... ### works? Even better.

phil294 commented 2 years ago

I'm wondering if there's value in adding a vscode command to insert a coffeesense / coffescript jsdoc comment stub. I keep forgetting the correct syntax and always come back to this github issue to remember.

@surjikal I think it depends on where you use a comment block - sometimes ``### works, sometimes #\n###, sometimes a lone ###.

I haven't found a one-fits-all solution yet unfortunately (both in front of variables and object properties), which is why the extension can't do it for you. But maybe there is. If you have found one and worked with it for a while and it works reliably, please let me know! Else we'll just have to wait for someone to tackle the CoffeeScript compiler enhancements.

phil294 commented 1 year ago

As things have gotten stale around here, I have had another look into this from the extension's point of view and fixed it there now. CoffeeSense now automatically transforms all of your

### something ###

into

#
### something ###

, also multi-line comment blocks, so you don't have to do that or the ``### thing anymore when you're working with ###* @... ### kind of JSDoc elements. I've noticed some of you do that, so you might want to update your code now! (unless you're also working with an external pipeline of course)

I wanted to avoid doing this as adding lines led to me having to adjust source maps awkwardly and everything was more prone to errors. Luckily, the large amount of tests helped to iron it all out.

On a related note (since we're on issue #1 here :-), I think this extension is now reasonably complete, so I will adjust the Readme a bit to reflect that - at least personally I don't plan on adding any more features.

surjikal commented 1 year ago

Thanks for the hard work, it's a really great extension!

surjikal commented 1 week ago

Hm.. for some reason, the

###*
@typedef {foo} bar
###

has stopped working recently. Using the backticks make it work though:

``###* 
@typedef {foo} bar
###

Looking at the compiled source produced by coffeesense, without the backticks, the comments are being stripped out. I'll post back here if I find a workaround.

phil294 commented 1 week ago

Hi @surjikal, that's strange as I haven't pushed an update to CoffeeSense in a long time. So I guess you found a bug that has been there all along. If you can narrow this down to a smallest possible reproducable single file snippet, maybe it'll be well fixable.