Closed Bug 1273623 Opened 3 years ago Closed 3 years ago

Don't run ESLint module check during regular invocation

Categories

(Firefox Build System :: Lint and Formatting, defect)

defect
Not set

Tracking

(firefox49 fixed)

RESOLVED FIXED
mozilla49
Tracking Status
firefox49 --- fixed

People

(Reporter: jryans, Assigned: miker)

Details

Attachments

(1 file, 1 obsolete file)

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?
: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)
(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)
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+
(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+
https://hg.mozilla.org/mozilla-central/rev/cee3b4a96f41
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Product: Testing → Firefox Build System
You need to log in before you can comment on or make changes to this bug.