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: added display-errors & display-sections option #33

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Gcaufy
Copy link
Contributor

@Gcaufy Gcaufy commented Dec 1, 2019

Checklist
  • npm test passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
Description of change

Similar to this PR: #30

1. Added displayErrors option

I added a displayErrors option, (like php display_errors). Because we can not say if env !== 'local' or env !== 'unittest' then env = 'prod', there are may test/uat...environments that we still need to show error stacks. I added this option so that we don't need to use env variable to see display errors or not. That's user behaviors. if they need to display errors in different env, then they can easily handle it in config files, like:

{
  displayErrors: process.env.NODE_ENV !== 'production'
}

or

{
  displayErrors (app) => app.env !== 'prod'
}

2. Added displaySections option

For some sections in error page, they are highly sensitive, like the appInfo section, or cookie section. Normally it contains the server side database password or user sensitive information. If we want to display errors in test/uat environment. then we should not to display those information to everybody. So I added this option. so that user can customize what section they want to show in the error pages

@codecov
Copy link

codecov bot commented Dec 1, 2019

Codecov Report

Merging #33 into master will increase coverage by 0.21%.
The diff coverage is 96.15%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master     #33      +/-   ##
=========================================
+ Coverage   96.98%   97.2%   +0.21%     
=========================================
  Files           5       5              
  Lines         166     179      +13     
=========================================
+ Hits          161     174      +13     
  Misses          5       5
Impacted Files Coverage Δ
config/config.default.js 100% <ø> (ø) ⬆️
app.js 97.18% <100%> (+0.12%) ⬆️
lib/error_view.js 96.59% <95.45%> (+0.43%) ⬆️

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 55d27e6...8b550b8. Read the comment docs.

@atian25 atian25 requested a review from okoala December 9, 2019 01:48
Copy link
Member

@hyj1991 hyj1991 left a comment

Choose a reason for hiding this comment

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

+1

@hyj1991 hyj1991 mentioned this pull request Dec 10, 2021
4 tasks
@hyj1991
Copy link
Member

hyj1991 commented Dec 10, 2021

这个 PR 的逻辑没问题,而且自定义看起来也是一个需求的点,但是形式上是使用允许自定义 isProd 函数还是额外增加 displayErrors 的配置需要从设计上考虑下,cc @atian25

Reference:#30

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants