Don't run ESLint module check during regular invocation

RESOLVED FIXED in Firefox 49

Status

RESOLVED FIXED
2 years ago
7 months ago

People

(Reporter: jryans, Assigned: miker)

Tracking

unspecified
mozilla49

Firefox Tracking Flags

(firefox49 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

2 years ago
Bug 1265082 adds some diagnostics to ESLint that checks the module setup on every run in a function "eslintModuleHasIssues".

This seems nice to do, but it really slows things down for me.  I just did a timed run and it took 37 seconds to complete (that's just the module check before ESLint is even called).

Can we move this outside of regular ESLint runs and over to some kind "--doctor" or "--diagnose" option?
(Reporter)

Comment 1

2 years ago
:mratcliffe, what do you think about this?
Flags: needinfo?(mratcliffe)
The check is fairly important now as we are using shrinkwrapped (version locked) packages and going between Firefox versions can require different versions.

That is a long time though... I can have a look how this can be sped up.

There are multiple lookups so maybe I can do that all in one and maybe use depth=0. Maybe we could have one quick check and one "tell me all my problems" check.

I will take a look.
Assignee: nobody → mratcliffe
Flags: needinfo?(mratcliffe)
Created attachment 8753818 [details] [diff] [review]
optimise-eslintModuleHasIssues.diff

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #0)
> This seems nice to do, but it really slows things down for me.  I just did a
> timed run and it took 37 seconds to complete (that's just the module check
> before ESLint is even called).
 
It only took 6 seconds for me. This optimized version only takes 0.38s though.

Turns out that `npm ls` is not overly optimal... especially in some versions of npm.

Instead of using npm ls to get the versions we are now directly reading the version numbers from each modules package.json.
Attachment #8753818 - Flags: review?(jryans)
(Reporter)

Comment 4

2 years ago
Comment on attachment 8753818 [details] [diff] [review]
optimise-eslintModuleHasIssues.diff

Review of attachment 8753818 [details] [diff] [review]:
-----------------------------------------------------------------

Yes, this is much much better, thanks!

::: python/mach_commands.py
@@ +341,5 @@
>  
>          return True
>  
>      def eslintModuleHasIssues(self):
>          print("Checking eslint and modules...")

I think it would make sense to use the normal mach logger for this message, so it has a timestamp like all the others.  Looking at the `eslint` command, maybe something like:

self.log(logging.INFO, 'eslint', {}, 'Checking eslint and modules...')
Attachment #8753818 - Flags: review?(jryans) → review+
Created attachment 8753896 [details] [diff] [review]
optimise-eslintModuleHasIssues.diff

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #4)
> Comment on attachment 8753818 [details] [diff] [review]
> optimise-eslintModuleHasIssues.diff
> 
> Review of attachment 8753818 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Yes, this is much much better, thanks!
> 
> ::: python/mach_commands.py
> @@ +341,5 @@
> >  
> >          return True
> >  
> >      def eslintModuleHasIssues(self):
> >          print("Checking eslint and modules...")
> 
> I think it would make sense to use the normal mach logger for this message,
> so it has a timestamp like all the others.

I decided to remove it.
Attachment #8753818 - Attachment is obsolete: true
Attachment #8753896 - Flags: review+
Keywords: checkin-needed

Comment 7

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/cee3b4a96f41
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox49: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla49

Updated

7 months ago
Product: Testing → Firefox Build System
You need to log in before you can comment on or make changes to this bug.