Forem: Lint issues with JavaScript Files

[nickytonline]

Describe the bug
This compliments work being done in #2470.

The *.js files inside app/javascript and app/assets/javascripts still have plenty of lint issues

To Reproduce
Steps to reproduce the behavior:

From the command line in the app/javascript folder, run eslint . --ext .js
From the command line in the app/assets/javascripts folder, run eslint . --ext .js

Expected behavior
There shouldn't be any lint issues.

Time to do some cleanup 😆

[Amorpheuz]

I would love to help out with this! I assume it will be much cleaner/smoother to fix a file, create a PR and then move on to the next, or should each fix be a commit in a PR?

[nickytonline]

Just create a PR and link it to this issue. Thanks for helping! 👏

[Jer-Sch]

Do you still need help with this? I'd love to take on some refactoring.

[rhymes]

Hi @palledorous, please go ahead, I'm going to copy my recommendation from the other PR:

to make reviews easier and also curb the possibility of introducing bugs, is not to tackle too many files at the same time.

I went ahead and grouped the results of yarn run eslint app/assets/javascripts --ext ".js" and yarn run eslint app/javascript --ext ".js":

• [ ] /app/assets/javascripts/initializers/initializeAllFollowButts.js (assigned to @uchihamalolan, @zachdixon is working on it too #4185)
• [x] /app/assets/javascripts/initializers/initializeAllTagEditButtons.js (assigned to @netochaves - https://github.com/thepracticaldev/dev.to/pull/4414)
• [x] /app/assets/javascripts/initializers/initializeArchivedPostFilter.js (assigned to @netochaves - https://github.com/thepracticaldev/dev.to/pull/4414)
• [x] /app/assets/javascripts/initializers/initializeArticleDate.js (assigned to @netochaves - https://github.com/thepracticaldev/dev.to/pull/4414)
• [x] /app/assets/javascripts/initializers/initializeArticleReactions.js (completed by @Amorpheuz, @zachdixon is working on it too #4185)
• [x] /app/assets/javascripts/initializers/initializeBaseUserData.js (assigned to @Amorpheuz, @dhanoz is working on it in https://github.com/thepracticaldev/dev.to/pull/4612)
• [x] /app/assets/javascripts/initializers/initializeBodyData.js (completed by @palledorous - https://github.com/thepracticaldev/dev.to/pull/4267)
• [x] /app/assets/javascripts/initializers/initializeCommentDate.js (completed by @Amorpheuz - https://github.com/thepracticaldev/dev.to/pull/4319)
• [x] /app/assets/javascripts/initializers/initializeCommentDropdown.js (completed by @Amorpheuz - https://github.com/thepracticaldev/dev.to/pull/4318)
• [x] /app/assets/javascripts/initializers/initializeCommentPreview.js (completed by @Amorpheuz - https://github.com/thepracticaldev/dev.to/pull/4320)
• [x] /app/assets/javascripts/initializers/initializeCreditsPage.js (completed by @sagarchoudhary96 - https://github.com/thepracticaldev/dev.to/pull/4312)
• [x] /app/assets/javascripts/initializers/initializeDashboardSort.js (completed by @sagarchoudhary96 - https://github.com/thepracticaldev/dev.to/pull/4313)
• [ ] /app/assets/javascripts/initializers/initializeDrawerSliders.js (assigned to @sagarchoudhary96 - https://github.com/thepracticaldev/dev.to/pull/4316)
• [ ] /app/assets/javascripts/initializers/initializePodcastPlayback.js (assigned to @netochaves - https://github.com/thepracticaldev/dev.to/pull/4358)
• [x] /app/assets/javascripts/initializers/initializeSettings.js (completed by @Amorpheuz - https://github.com/thepracticaldev/dev.to/pull/4329)
• [ ] /app/assets/javascripts/initializers/initializeSwipeGestures.js (@zachdixon is working on it too #4185)
• [ ] /app/assets/javascripts/initializers/initializeTimeFixer.js (assigned to @netochaves - https://github.com/thepracticaldev/dev.to/pull/4416)
• [ ] /app/assets/javascripts/initializers/initializeTouchDevice.js (assigned to @netochaves - #4415)
• [ ] /app/assets/javascripts/initializers/initNotifications.js (assigned to @netochaves - https://github.com/thepracticaldev/dev.to/pull/4355)
• [ ] /app/assets/javascripts/lib/pulltorefresh.js (assigned to @nickytonline)
• [x] /app/assets/javascripts/utilities/sendFetch.js (completed by @ludamillion - https://github.com/thepracticaldev/dev.to/pull/4363)
• [x] app/javascript/packs/notificationSubscriptionHandler.js (completed by @blascsi - https://github.com/thepracticaldev/dev.to/pull/4172)

you're welcome to take on any of them!

[uchihamalolan]

I'll try to refactor initializeAllFollowButts.js

[blascsi]

I have already made a PR for app/javascript/packs/notificationSubscriptionHandler.js! :)

[netochaves]

Can we use arrow functions instead of function() ?

[nickytonline]

Yes you can use arrow functions and I believe the lint rules will prefer it in the case of anonymous functions.

[netochaves]

Yeah, thanks

[nickytonline]

Do you still need help with this? I'd love to take on some refactoring.

@palledorous, I believe there is still work to be done in here. Just maybe sync up with the others in this thread to see which files they're working on.

[Jer-Sch]

Sounds good!

[netochaves]

https://github.com/thepracticaldev/dev.to/blob/66faaccdd894a75887e6e3dfad30a3152e18a4fb/app/assets/javascripts/initializers/initNotifications.js#L48
This function maybe is missing an import from /app/assets/javascripts/utilities/checkUserLoggedIn.js?

[nickytonline]

https://github.com/thepracticaldev/dev.to/blob/66faaccdd894a75887e6e3dfad30a3152e18a4fb/app/assets/javascripts/initializers/initNotifications.js#L48

This function maybe is missing an import from /app/assets/javascripts/utilities/checkUserLoggedIn.js?

@netochaves, in the older part of the frontend code base, e.g. app/assets/javascripts/*, some functions are loaded in other files. There are no ES Modules in this part of the code base.

In this case, that function is defined here, https://github.com/thepracticaldev/dev.to/blob/66faaccdd894a75887e6e3dfad30a3152e18a4fb/app/assets/javascripts/utilities/checkUserLoggedIn.js. eslint is probably complaining about this function not being defined, so you will need to add checkUserLoggedIn to the eslint globals configuration, https://github.com/thepracticaldev/dev.to/blob/66faaccdd894a75887e6e3dfad30a3152e18a4fb/app/javascript/.eslintrc.js#L27

[netochaves]

Do you mean here? https://github.com/thepracticaldev/dev.to/blob/66faaccdd894a75887e6e3dfad30a3152e18a4fb/app/assets/javascripts/.eslintrc.js#L14

[nickytonline]

Do you mean here?

https://github.com/thepracticaldev/dev.to/blob/66faaccdd894a75887e6e3dfad30a3152e18a4fb/app/assets/javascripts/.eslintrc.js#L14

Yes, that is the file to add it to. I referenced the wrong eslint configuration file. 🙃 More info on eslint globals here, https://eslint.org/docs/user-guide/configuring#specifying-globals

[rhymes]

You can also declare globals inline in the file, since right now basically all functions in app/assets/javascripts are global, I'm not sure how worth it is to use the globals inside the eslint file :D

[nickytonline]

Thanks to everyone that has been working on this issue. Happy Hacktoberfest! 👏🔥

[rhymes]

@palledorous I've added your PR to the list up there!

[Amorpheuz]

I would like to take up /app/assets/javascripts/initializers/initializeArticleReactions.js

[Amorpheuz]

I want to take up initializeCommentDate.js, initializeCommentDropdown.js, initializeCommentPreview.js too next!

P.S. thanks rhymes for going through all PRs and marking all the files! :smiley: I did go through those but it seems I missed initializeArticleReactions in zach's PR, my bad.

[sagarchoudhary96]

@rhymes i would like to take up /app/assets/javascripts/utilities/sendFetch.js file

[sagarchoudhary96]

looks like sendFetch don't have any eslint error.

[sagarchoudhary96]

i will take up /app/assets/iinitializer/initializeCreditsPage.js

[sagarchoudhary96]

raised PR for it @rhymes . taking up initializeDashboardSort from app/assets/javascripts.

[rhymes]

@sagarchoudhary96 if I run yarn eslint app/assets/javascripts/utilities/sendFetch.js I see an issue:

app/assets/javascripts/utilities/sendFetch.js
  10:9  error  Parsing error: Unexpected token ..

probably because the spread operator is not allowed in our configuration of eslintrc

[sagarchoudhary96]

@rhymes , yeah that the only one but i was reading about it, we will then need to make changes in eslintrc file for it.

[rhymes]

@sagarchoudhary96 I would replace the spread operator TBH, changing the eslintrc means that we might introduce new lint errors (for example by bumping the version of JavaScript) in all files

[sagarchoudhary96]

also @rhymes , in app/assets/intializers/initializeDrawerSliders.js we are using function from other files for e.g initializeSwipeGestures() which is defined in initializeSwipeGestures.js file. so should i add import statements for those or leave them ?

[rhymes]

@sagarchoudhary96 you can declare, inside initializeDrawerSliders.js those functions as global, with a statement like this at the top of the file: /* global initializeSwipeGestures */

[nickytonline]

I have a PR that fixes the issues with /app/assets/javascripts/lib/pulltorefresh.js coming up.

[Amorpheuz]

I would like to grab /app/assets/javascripts/initializers/initializeSettings.js

[Amorpheuz]

I would love to take /app/assets/javascripts/initializers/initializeBaseUserData.js!

[ludamillion]

@sagarchoudhary96 I would replace the spread operator TBH, changing the eslintrc means that we might introduce new lint errors (for example by bumping the version of JavaScript) in all files

I attempted to replace the spread operator by using the Object.assign approach. This resulted in a new lint error indicating that Object.assign is deprecated and should be replaced with the spread operator 😜. However I bumped the ecmaVersion in the app/assets/javascript directory and it did not result in any new lint errors.

[Jer-Sch]

@ludamillion I was looking into that locally myself earlier tonight and noticed the same thing. I think it may have even resolved several of the errors!

[dhanoz]

I would like to take on /bin/rails !

[rhymes]

@dhanoz this issue is about JavaScript files :)

[3b3ziz]

I would like to take /app/assets/javascripts/initializers/initializePodcastPlayback.js.

[rhymes]

@3b3ziz sorry, we forgot to update the list, that file is already tackled on here https://github.com/thepracticaldev/dev.to/pull/4358

[rhymes]

@nickytonline are you still working on pulltorefresh.js ?

[nickytonline]

No, I completely got sidetracked with other PRs and completely forgot about it. Feel free to grab it @3b3ziz.

[dhanoz]

@rhymes
Can I make a PR on /app/assets/javascripts/initializers/initializeTouchDevice.js?
I have fixed some of the lint errors in it!

[netochaves]

@dhanoz This was already done at #4415

[dhanoz]

Can I take on to /app/assets/javascripts/initializers/initializeBaseUserData.js? @netochaves

[nickytonline]

Just updating the list of lint errors here as it's a lot smaller now.

app/assets/javascripts/initializers/initNotifications.js
   49:5   error  'checkUserLoggedIn' is not defined  no-undef
   52:9   error  'instantClick' is not defined       no-undef
   53:7   error  'InstantClick' is not defined       no-undef
   55:9   error  'InstantClick' is not defined       no-undef
   72:11  error  'sendHapticMessage' is not defined  no-undef
   98:11  error  'butts' is already defined          no-redeclare
   99:16  error  'i' is already defined              no-redeclare
  100:13  error  'butt' is already defined           no-redeclare

app/assets/javascripts/initializers/initializeAllFollowButts.js
   84:5  error  'showModal' is not defined       no-undef
  132:3  error  'shouldNotFetch' is not defined  no-undef
  154:5  error  'showModal' is not defined       no-undef

app/assets/javascripts/initializers/initializeSwipeGestures.js
   3:3  error  'swipeState' is not defined    no-undef
  40:7  error  'swipeState' is not defined    no-undef
  41:5  error  'swipeState' is not defined    no-undef
  42:5  error  'slideSidebar' is not defined  no-undef
  45:5  error  'swipeState' is not defined    no-undef
  46:5  error  'slideSidebar' is not defined  no-undef
  53:7  error  'swipeState' is not defined    no-undef
  54:5  error  'swipeState' is not defined    no-undef
  55:5  error  'slideSidebar' is not defined  no-undef
  58:5  error  'swipeState' is not defined    no-undef
  59:5  error  'slideSidebar' is not defined  no-undef

If you need to check this list you can run the following from your local, node_modules/.bin/eslint app/javascript/**/*.js.

[Amorpheuz]

I would like to pick up app/assets/javascripts/initializers/initializeSwipeGestures.js!

P.S. I think, in order to check the list, one needs to run: node_modules/.bin/eslint app/assets/javascripts/**/*.js 😅

[nickytonline]

All remaining lint issues in the frontend have been fixed. Now you will not be able to pass a build if you commit lint errors thanks to #9100 .