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

feat(vue-renderer/vue-app): report SSR console logs to the browser with consola #5673

Merged
merged 8 commits into from
May 9, 2019

Conversation

atinux
Copy link
Member

@atinux atinux commented May 8, 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

This feature is for development mode to improve the developer experience while developing.

Problem

Switching and looking at the terminal when you want to debug your Nuxt app by doing some console.log can be annoying. This can disturb newcomers to universal rendering.

Solution

In dev only, it copies the SSR logs to the browser and display them before mounting the Vue app.

This is how it look like when adding some logs to examples/hello-world/pages/index.vue:

asyncData() {
  console.log('Log')
  console.warn('Warning')
  console.error('Error')
  console.error(new Error('Aie'))
}

Result:
Screenshot 2019-05-08 at 17 10 11

Checklist:

@clarkdo I would like your insight regarding the tests and the best place to test this feature.

@atinux atinux requested a review from a team May 8, 2019 15:19
@codecov-io
Copy link

codecov-io commented May 8, 2019

Codecov Report

Merging #5673 into dev will decrease coverage by 0.09%.
The diff coverage is 72.72%.

Impacted file tree graph

@@            Coverage Diff            @@
##              dev    #5673     +/-   ##
=========================================
- Coverage   95.67%   95.57%   -0.1%     
=========================================
  Files          81       81             
  Lines        2635     2645     +10     
  Branches      671      673      +2     
=========================================
+ Hits         2521     2528      +7     
- Misses         96       98      +2     
- Partials       18       19      +1
Impacted Files Coverage Δ
packages/vue-renderer/src/renderers/ssr.js 92.15% <72.72%> (-5.41%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2fd05e0...a1fc458. Read the comment docs.

galvez
galvez previously approved these changes May 8, 2019
Copy link

@galvez galvez left a comment

Choose a reason for hiding this comment

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

YES! So simple, very nicely done @atinux :)

@galvez
Copy link

galvez commented May 8, 2019

@atinux perhaps this could be an opt-in feature?

kevinmarrec
kevinmarrec previously approved these changes May 8, 2019
Copy link
Contributor

@kevinmarrec kevinmarrec left a comment

Choose a reason for hiding this comment

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

Great stuff @atinux, well done !

Copy link
Member

@clarkdo clarkdo left a comment

Choose a reason for hiding this comment

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

Do you think if this feature needs an extra option ? Maybe sth like: server.console: true|false|'info'|'warn'|'error'

If there is any user who is using dev mode in production or integration testing environment, this may be a risk for sensitive information leak.

About the test, I think it could be in fixture like base, ssr or with-config, and verify NUXT.logs in jsdom or html string

@atinux
Copy link
Member Author

atinux commented May 8, 2019

I added a PR to consola to support console.warn on the browser (unjs/consola#49).

@clarkdo I am not up to add another option for this, it's like for the loading-screen in dev.
They should not run nuxt dev in production in any way.
Thanks for the tests I will look into it 😄

@atinux atinux dismissed stale reviews from kevinmarrec and galvez via cb0cbc9 May 8, 2019 15:54
@clarkdo
Copy link
Member

clarkdo commented May 8, 2019

@atinux Another thing comes into my mind is: Do we need to support messages from console as well ?

consola has already wrapped console function in dev mode

packages/vue-app/template/server.js Outdated Show resolved Hide resolved
packages/vue-app/template/server.js Outdated Show resolved Hide resolved
packages/vue-app/template/server.js Outdated Show resolved Hide resolved
@pi0 pi0 changed the title feat(vue-app): Copy logs during server-rendering to the browser with consola feat(vue-app): report SSR console logs to the browser with consola May 8, 2019
@pi0
Copy link
Member

pi0 commented May 8, 2019

consola updated to 2.6.1 (#5674)

pi0
pi0 previously approved these changes May 8, 2019
@atinux atinux changed the title feat(vue-app): report SSR console logs to the browser with consola feat(vue-renderer/vue-app): report SSR console logs to the browser with consola May 8, 2019
@atinux
Copy link
Member Author

atinux commented May 8, 2019

I could not find a quick way to check for logs since they are mocked in testing and catched by Jest.

@atinux atinux requested review from pi0 and clarkdo and removed request for pi0 May 8, 2019 23:09
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.

7 participants