Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(cli): show memory usage after build for nuxt dev #5514

Merged
merged 6 commits into from Apr 12, 2019
Merged

Conversation

pimlie
Copy link

@pimlie pimlie commented Apr 11, 2019

Types of changes

  • Bug fix (a non-breaking change which fixes an issue)
  • New feature (a non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Description

As reported by @koresar in #5509, the memory usage is printed too early for the dev command. This pr makes it optional to print memory usage in the banner and adds a simple show mem usage method for printing with consola.info

Checklist:

  • My change requires a change to the documentation.
  • I have updated the documentation accordingly. (PR: #)
  • I have added tests to cover my changes (if not applicable, please state why)
  • All new and existing tests are passing.

@koresar
Copy link

koresar commented Apr 11, 2019

Thank you. I appreciate!

packages/cli/src/utils/banner.js Show resolved Hide resolved
packages/cli/src/commands/dev.js Outdated Show resolved Hide resolved
Copy link
Member

@pi0 pi0 left a comment

Choose a reason for hiding this comment

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

Please change it to show usage after renderer loaded resources

@pimlie
Copy link
Author

pimlie commented Apr 12, 2019

render:resourcesLoaded is called twice and render:done is never called, will have a look at your other suggestion of adding another hook

@pimlie
Copy link
Author

pimlie commented Apr 12, 2019

This is getting way too complicated for a simple and not precise feature anyway, but I cant get a 'render:resourcesDone' hook as proposed by @pi0 to work because isModernReady already returns true on the first call to build:resources from webpack/builder which causes the new resourcesDone hook still to be called twice.

I can fix it probably but then I need to change the isReady / isModernReady behaviour in renderer so they only return true when their respective build:resources hook has been called from webpack/builder. Or do you have a better idea?

Copy link
Member

@Atinux Atinux left a comment

Choose a reason for hiding this comment

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

I like the idea of showing the correct memory usage in dev, thanks for the PR <3

Actually we need to wait for the renderer to be ready (since the bundleRenderer will take some memory usage).

We need indeed to add a new hook for this, I believe together we will find a nice way to do so :)

packages/cli/src/utils/memory.js Outdated Show resolved Hide resolved
packages/cli/src/utils/memory.js Outdated Show resolved Hide resolved
Sébastien Chopin and others added 2 commits April 12, 2019 21:00
Co-Authored-By: pi0 <pyapar@gmail.com>
Co-Authored-By: pi0 <pyapar@gmail.com>
pi0
pi0 previously approved these changes Apr 12, 2019
@@ -5,7 +5,7 @@ import prettyBytes from 'pretty-bytes'
export function getMemoryUsage() {
// https://nodejs.org/api/process.html#process_process_memoryusage
const { heapUsed, rss } = process.memoryUsage()
return `Memory usage: ${chalk.bold(prettyBytes(heapUsed))} (RSS: ${prettyBytes(rss)})`
return { heap: heapUsed, rss }
Copy link
Member

Choose a reason for hiding this comment

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

This is breaking banner.js

@pi0 pi0 changed the title fix(cli): show memory usage after build for dev cmd fix(cli): show memory usage after build for nuxt dev Apr 12, 2019
@pi0 pi0 merged commit 19fbbb6 into dev Apr 12, 2019
@pi0 pi0 mentioned this pull request Apr 12, 2019
@pimlie pimlie deleted the feat-memory-usage branch April 12, 2019 17:30
@danielroe danielroe added the 2.x label Jan 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants