[SMS] fix remain jsdoc error

RESOLVED FIXED

Status

Firefox OS
Gaia::SMS
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: gasolin@mozilla.com, Assigned: gioyik, Mentored)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: good-first-bug, zh)

Attachments

(2 attachments)

(Reporter)

Description

3 years ago
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

3 years ago
hey! i would like to do this bug.
can you please give me some pointers for starting.
(Reporter)

Comment 2

3 years ago
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.
Assignee: nobody → anubhav.worklinux
Status: NEW → ASSIGNED
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 :/
(Reporter)

Comment 4

3 years ago
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.

Updated

3 years ago
Assignee: anubhav.worklinux → nobody
Status: ASSIGNED → NEW

Comment 5

3 years ago
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!
(Reporter)

Comment 6

3 years ago
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
You need at least node 0.10 but npm 2 (which is not the one bundled in node).

Comment 8

3 years ago
Created attachment 8635448 [details]
jsdoc:sms error

Comment 9

3 years ago
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
I think you can enable a more verbose output while generating the jsdoc.
(Reporter)

Comment 11

3 years ago
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.
(Assignee)

Comment 12

3 years ago
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.
Flags: needinfo?(gasolin)
(Assignee)

Comment 13

3 years ago
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.
(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
(Assignee)

Comment 15

3 years ago
> 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?
(Reporter)

Comment 16

3 years ago
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.
Flags: needinfo?(gasolin)
(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)
(Assignee)

Comment 18

3 years ago
> 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.
(Reporter)

Comment 19

3 years ago
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?
Flags: needinfo?(gioyik)
Created attachment 8644817 [details] [review]
[gaia] Gioyik:bug-1170464 > mozilla-b2g:master
(Assignee)

Comment 21

3 years ago
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
Flags: needinfo?(gioyik)
(Assignee)

Updated

3 years ago
Attachment #8644817 - Flags: review?(gasolin)
(Assignee)

Comment 22

3 years ago
Let me know if the fixes are correct.
Assignee: nobody → gioyik
Added some comments on github.
(Assignee)

Comment 24

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

Thanks Julien.
(Reporter)

Comment 25

3 years ago
there's a lint error in  apps/sms/views/shared/js/utils.js, please fix it
(Assignee)

Comment 26

3 years ago
Fixed!

Should be ready to merge it now.
(Reporter)

Comment 27

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

looks good to me, thanks you for contribution!
Attachment #8644817 - Flags: review?(gasolin) → review+
(Assignee)

Comment 28

3 years ago
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
(Reporter)

Comment 29

3 years ago
merged https://github.com/mozilla-b2g/gaia/commit/934c7ca2176cde32d47de3c12020a5dacefc8047
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.