Skip to content

feat(kit, schema): measure module setup timings#18648

Merged
danielroe merged 11 commits into
mainfrom
feat/module-install-meta
Mar 10, 2023
Merged

feat(kit, schema): measure module setup timings#18648
danielroe merged 11 commits into
mainfrom
feat/module-install-meta

Conversation

@pi0

@pi0 pi0 commented Jan 31, 2023

Copy link
Copy Markdown
Member

🔗 Linked issue

resolves #15739

❓ Type of change

  • 📖 Documentation (updates to the documentation or readme)
  • 🐞 Bug fix (a non-breaking change that fixes an issue)
  • 👌 Enhancement (improving an existing functionality like performance)
  • ✨ New feature (a non-breaking change that adds functionality)
  • ⚠️ Breaking change (fix or feature that would cause existing functionality to change)

📚 Description

  • Adds new ModuleReturn interface for Nuxt modules with timings subkey
  • Automatically measure kit-defined modules setup time and store in _installedModules
  • [DX] Show a warning message if module takes more than 1 seconds to initialize
 WARN  Module test took 1502.4ms to setup!  

📝 Checklist

  • I have linked an issue or discussion.
  • I have updated the documentation accordingly.

@codesandbox

codesandbox Bot commented Jan 31, 2023

Copy link
Copy Markdown

CodeSandbox logoCodeSandbox logo  Open in CodeSandbox Web Editor | VS Code | VS Code Insiders

@pi0 pi0 requested review from antfu, atinux and danielroe January 31, 2023 17:20
Comment thread packages/kit/src/module/define.ts Outdated
Co-authored-by: Sébastien Chopin <seb@nuxt.com>
@atinux

atinux commented Jan 31, 2023

Copy link
Copy Markdown
Member

It seems that using an anonymous module in the config does not trigger a warn:

export default defineNuxtConfig({
  modules: [
    async () => {
      // wait 2 sec
      await new Promise(resolve => setTimeout(resolve, 2000))
    }
  ]
})

I guess it's safe to ignore but we should advice to use @nuxt/kit to define Nuxt modules:

import { defineNuxtModule } from '@nuxt/kit'

export default defineNuxtConfig({
  modules: [
    defineNuxtModule({
      meta: {
        name: 'my-module'
      },
      async setup () {
        // wait 2 sec
        await new Promise(resolve => setTimeout(resolve, 2000))
      }
    })
  ]
})

CleanShot 2023-01-31 at 18 34 45@2x

@atinux atinux left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me! Waiting for @danielroe review as well and @antfu regarding devtools integration

@TheAlexLichter TheAlexLichter left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Should we document it somewhere (besides auto-generated docs)?

@atinux

atinux commented Jan 31, 2023

Copy link
Copy Markdown
Member

I guess in #18551

Comment thread packages/schema/src/types/module.ts Outdated
Comment thread packages/kit/src/module/define.ts Outdated
Comment thread packages/kit/src/module/define.ts Outdated
@danielroe danielroe added 3.x ✨ enhancement New feature or improvement to existing functionality labels Feb 1, 2023
@harlan-zw harlan-zw requested review from harlan-zw and removed request for harlan-zw February 7, 2023 03:59
@harlan-zw

harlan-zw commented Feb 7, 2023

Copy link
Copy Markdown
Contributor

PR looks good overall for timing. My two cents:

  1. Utilise debug output

Consider the debug output atm.

image

Context is lost without knowing what module setup is taking place. It would be good to add a debug on the start of module setup and on the finish. There are a couple of modules that do globs and they have a habit of accidentally scanning the entire node_modules and stalling indefinitely. (pinceau, windicss, etc). Or making network requests which are using large timeouts.

  1. Warning for slow modules

I think this just encourages module authors to "hide" logic that takes more than 1s within hooks, making it hard to debug, so I am against this (unless debug is enabled).

@danielroe danielroe added this to the v3.3 milestone Feb 15, 2023
@atinux

atinux commented Mar 2, 2023

Copy link
Copy Markdown
Member

Completely agree with you on this @harlan-zw

@danielroe

Copy link
Copy Markdown
Member

@pi0 feel free to let me know if I can be helpful here 🙏

@danielroe danielroe mentioned this pull request Mar 8, 2023
@pi0

pi0 commented Mar 10, 2023

Copy link
Copy Markdown
Member Author

I have made a few changes. And also increased the minimum amount to 5 seconds which is unlikely to happen unless a module is doing something really wrong in which case, "end user" should be aware of why their development experience is slow. And also default hook logs, we have explicit module name as part of warning message so it is tracable.

I think anyway using this new exported meta devtools can give better experience. And we might try it for a while across ecosystem to first find the evil and decide to not warn if there are modules that absolutely cannot be optimized! But without a small push to the ecosystem and user awareness by testing against different environments, it is unlikely we could ever find those kind of modules.

@danielroe

Copy link
Copy Markdown
Member

This approach seems good to me.

🚨 Note to readers: I think it is not too strong to say we are now engaged in a fight against evil across the galaxy. 🚨

@danielroe danielroe requested a review from antfu March 10, 2023 10:25
@danielroe danielroe merged commit 6bd9b94 into main Mar 10, 2023
@danielroe danielroe deleted the feat/module-install-meta branch March 10, 2023 11:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

3.x ✨ enhancement New feature or improvement to existing functionality

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Track install time for modules

6 participants