Skip to content
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

Positioning of JSDoc comments #1

Closed
phil294 opened this issue Aug 18, 2021 · 17 comments
Closed

Positioning of JSDoc comments #1

phil294 opened this issue Aug 18, 2021 · 17 comments

Comments

@phil294
Copy link
Owner

phil294 commented Aug 18, 2021

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
Copy link

robert-boulanger commented Jan 23, 2022

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
Copy link
Owner Author

phil294 commented Jan 23, 2022

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

@robert-boulanger
Copy link

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
Copy link
Owner Author

phil294 commented Jan 25, 2022

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
Copy link

@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
Copy link

STRd6 commented Apr 18, 2022

Since the positioning of block comments was why this revert happened 0a147d1 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
Copy link
Owner Author

phil294 commented Apr 18, 2022

@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
Copy link

STRd6 commented Apr 18, 2022

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
Copy link

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 added a commit that referenced this issue Apr 26, 2022
now both logics combined, looks like it works fine.
ref #1
reverts a bit of 0a147d which had reverted c71f45
@phil294
Copy link
Owner Author

phil294 commented Apr 26, 2022

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
Copy link

STRd6 commented Apr 26, 2022

@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
Copy link

STRd6 commented Apr 28, 2022

Adding some links to CoffeeScript issues for reference:

jashkenas/coffeescript#5417
jashkenas/coffeescript#5415

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

phil294 added a commit that referenced this issue May 22, 2022
prefer ``###* ... ### syntax to #\n###* ... ###
ref #1
@phil294 phil294 reopened this May 22, 2022
@surjikal
Copy link

surjikal commented Jul 8, 2022

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
Copy link
Owner Author

phil294 commented Sep 4, 2022

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 phil294 closed this as completed in cad5151 Aug 6, 2023
@phil294
Copy link
Owner Author

phil294 commented Aug 6, 2023

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
Copy link

surjikal commented Aug 7, 2023

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

@surjikal
Copy link

surjikal commented Nov 7, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants