1170464 - [SMS] fix remain jsdoc error

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::SMS, defect)

Description

[[e-mail]]

Since bug 1151765 has landed and support es6 syntax for jsdoc parsing, there're still some syntax issues not resolved in gaia.

We could solve jsdoc issues per-app.

Comment 1

[anubhav.worklinux]

hey! i would like to do this bug.
can you please give me some pointers for starting.

Comment 2

[[e-mail]]

Thanks for interesting in solving this issue.

Refer to bug 1170456, first you need run `npm install` in gaia repo to get up-to-date packages. 

Then run command

`gulp jsdoc:calendar` to make sure everything works. 

You can remove `exclude` files from `jsdoc.json`, fix the file and run the command again to check if jsdoc is actually fixed. All of the warnings are not relevant, focus on solving errors.

Comment 3

[Julien Wajsberg [:julienw]]

Fred, can you be a little clearer about what the issues are ?

I don't want to change our working syntax just because jsdoc is lagging in ES6 support :/

Comment 4

[[e-mail]]

Julien, 

In most cases the error is caused by we does not follow jsdoc comment format correctly, not really related to the source code itself, so we will not need to change our working es6 syntax.

Comment 5

[anubhav.worklinux]

Hi Fred, I was unable to run npm install successfully on my system after so many attempts. i manually installed gulp but even that did not work.
So i reset the assignee to default for somebody else to work on this bug.
Thanks!

Comment 6

[[e-mail]]

Sorry to hear there's some build tool issue. I think its about node version. If you have time please try node 0.10.x and see if it works. 

You can manage different node version on your desktop via nvm https://github.com/creationix/nvm

Comment 7

[Julien Wajsberg [:julienw]]

You need at least node 0.10 but npm 2 (which is not the one bundled in node).

Comment 8

[anubhav.worklinux]

Attachment: jsdoc:sms error

Comment 9

[anubhav.worklinux]

Hi fred, finally ran npm install successfully.
Created attachment with the error i get on running jsdoc:sms.
I can't understand what the error really is.
Need Help.
Thanks

Comment 10

[Julien Wajsberg [:julienw]]

I think you can enable a more verbose output while generating the jsdoc.

Comment 11

[[e-mail]]

I ran `gulp jsdoc:sms` with the master in console, saw there is a long list of WARNING, and an ERROR message

ERROR: Unable to parse /Users/gasolin/Documents/web/foxbox/gaia/apps/sms/services/js/activity/activity_shim.js: Line 48: Unexpected token [

including fix this issue, remove `exclude` files from `jsdoc.json` should bring more ERRORs to fix.

Comment 12

[Giovanny Gongora [:gioyik]]

Fred,

What could be the solution for this ERROR:

ERROR: Unable to parse /Users/gasolin/Documents/web/foxbox/gaia/apps/sms/services/js/activity/activity_shim.js: Line 48: Unexpected token [

The line is this one: https://github.com/mozilla-b2g/gaia/blob/d4548b57fa210c6a449b674d2428bc733f32d45f/apps/sms/services/js/activity/activity_shim.js#L48

I tried with `this[priv.request]: null,` but still getting the same error.

Comment 13

[Giovanny Gongora [:gioyik]]

For example this one (after remove exclude files):

ERROR: Unable to parse /home/gioyik/gaia/apps/sms/views/shared/js/contacts.js: Line 210: Unexpected token =>

Could be this related to the lack of JSDoc support for ArrowFunctionExpression? Or maybe I am mixing topics. If I am right, this one will be solved after JSDoc adds full support to ES6 syntax.

Comment 14

[Julien Wajsberg [:julienw]]

(In reply to Giovanny Gongora [:gioyik] from comment #12)
> Fred,
> 
> What could be the solution for this ERROR:
> 
> ERROR: Unable to parse
> /Users/gasolin/Documents/web/foxbox/gaia/apps/sms/services/js/activity/
> activity_shim.js: Line 48: Unexpected token [
> 
> The line is this one:
> https://github.com/mozilla-b2g/gaia/blob/
> d4548b57fa210c6a449b674d2428bc733f32d45f/apps/sms/services/js/activity/
> activity_shim.js#L48

See https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Object_initializer#Computed_property_names for more informaiton about this new syntax. This is indeed a new syntax for ES6.

> 
> I tried with `this[priv.request]: null,` but still getting the same error.

Yeah because this is not a valid syntax :)



According to [1] they don't support arrow functions yet, not even in master. But I think it will come soon now that V8 (and thus node) will support it.

[1] https://github.com/jsdoc3/jsdoc/issues/555

Comment 15

[Giovanny Gongora [:gioyik]]

> Yeah because this is not a valid syntax :)

I tried it because they use `this[priv.request]: null` a few lines down: https://github.com/mozilla-b2g/gaia/blob/d4548b57fa210c6a449b674d2428bc733f32d45f/apps/sms/services/js/activity/activity_shim.js#L86

I got a little bit confused about when is not a valid syntax. I have to read more about ES6. :)

So, most of those problems with JSDoc will get solved after ES6 adoption?

Comment 16

[[e-mail]]

Nice catch. The bug Jsdoc fix is mainly about fixing /** jsdocs in comments */

For files with jsdoc unsupport features such as [priv.request]: null or the complex then(([]) => {}) syntax, I suggest leave them in exclude files and fix the others.

Comment 17

[Julien Wajsberg [:julienw]]

(In reply to Giovanny Gongora [:gioyik] from comment #15)
> > Yeah because this is not a valid syntax :)
> 
> I tried it because they use `this[priv.request]: null` a few lines down:

If you read carefully, this is `this[priv.request] = null`. This is perfectly fine ES5 :D (and even ES3 I think).
 
> So, most of those problems with JSDoc will get solved after ES6 adoption?


Yeah but jsdoc is not moving really quick here :/

(In reply to Fred Lin [:gasolin] from comment #16)
> 
> For files with jsdoc unsupport features such as [priv.request]: null or the
> complex then(([]) => {}) syntax, I suggest leave them in exclude files and
> fix the others.

There are a lot of files like this in the SMS app :(

Should we try other doc generators ? (like yuidoc, or docchi)

Comment 18

[Giovanny Gongora [:gioyik]]

> I suggest leave them in exclude files and fix the others.

If I am not wrong there are only 4 ERROR for SMS and are about ES6 syntax (not about jsdoc comments). So this bug will be solved after complete ES6 adoption.

> Should we try other doc generators ? (like yuidoc, or docchi)

What would be better, export docs from JSDoc to Docchi or wait for JSDoc to add full ES6 support? 

In my opinion, JSDoc is almost complete with ES6 support: https://github.com/jsdoc3/jsdoc/issues/555#issuecomment-31229000. But full support will be shipped only with v3.4 that still needs time (not sure how much, maybe months). I don't know how similar is the syntax between JSDoc and alternatives (yuidoc or docchi) to make less painful the export process. If the unique reason to change from JSDoc is ES6, I would wait for JSDoc full support, *but* if is really necessary (P1) right now take care of ES6 in docs, should consider an alternative soon.

Comment 19

[[e-mail]]

Since its might cause more debates & efforts to switch to some of not-that-active doc generators, I'd suggest leave those files with jsdoc unsupport syntax in exclude list, and fix the others that prevent `gulp jsdoc:sms` to work.

Giovanny, would you like to make a pull request for that?

Comment 20

[GitHub Autolander Bot]

Attachment: [gaia] Gioyik:bug-1170464 > mozilla-b2g:master

Comment 21

[Giovanny Gongora [:gioyik]]

I leave 2 files in exclude section, due errors are about ES6.

ERROR: Unable to parse /home/gioyik/gaia/apps/sms/views/shared/js/contacts.js: Line 210: Unexpected token =>
ERROR: Unable to parse /home/gioyik/gaia/apps/sms/services/js/activity/activity_shim.js: Line 48: Unexpected token [

I created a PR for this bug

Comment 22

[Giovanny Gongora [:gioyik]]

Let me know if the fixes are correct.

Comment 23

[Julien Wajsberg [:julienw]]

Added some comments on github.

Comment 24

[Giovanny Gongora [:gioyik]]

Changes are done. I squashed everything in one commit, so you can merge without problem.

Thanks Julien.

Comment 25

[[e-mail]]

there's a lint error in  apps/sms/views/shared/js/utils.js, please fix it

Comment 26

[Giovanny Gongora [:gioyik]]

Fixed!

Should be ready to merge it now.

Comment 27

[[e-mail]]

Comment on attachment 8644817 [details] [review]
[gaia] Gioyik:bug-1170464 > mozilla-b2g:master

looks good to me, thanks you for contribution!

Comment 28

[Giovanny Gongora [:gioyik]]

Thank you for the feedback and support. Let me know if there's something else to help with, I am looking for mentored bugs and more complex work than good first bugs. :)

Thanks

Comment 29

[[e-mail]]

merged https://github.com/mozilla-b2g/gaia/commit/934c7ca2176cde32d47de3c12020a5dacefc8047