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

[BREAKING CHANGE] feat: remove notfound.enableRedirect #368

Merged
merged 2 commits into from
Feb 13, 2017
Merged

[BREAKING CHANGE] feat: remove notfound.enableRedirect #368

merged 2 commits into from
Feb 13, 2017

Conversation

dead-horse
Copy link
Member

@dead-horse dead-horse commented Feb 13, 2017

Checklist
  • npm test passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

notfound

Description of change

remove notfound.enableRedirect, always redirect if notfound.pageUrl present, ensure has the same behavior in different environments.

@dead-horse dead-horse added this to the 1.0.0 milestone Feb 13, 2017
@mention-bot
Copy link

@dead-horse, thanks for your PR! By analyzing the history of the files in this pull request, we identified @fengmk2, @atian25 and @popomore to be potential reviewers.

@dead-horse
Copy link
Member Author

there's no additional informations we can present in non-production environments, so keep it the same as production environment is more reasonable.

@@ -18,16 +18,11 @@ module.exports = options => {
return;
}

if (options.enableRedirect && options.pageUrl) {
if (options.pageUrl) {
Copy link
Member

Choose a reason for hiding this comment

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

还要防止死循环,确保 referer 不是自己,免得开发者配置了 options.pageUrl ='/404',然后又没有实现 404 页面

Copy link
Member Author

Choose a reason for hiding this comment

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

比较难,例如 referrer 为 http://dev.xxx.com/404,没法判断它是自己,这个还是交由开发者自己保证吧,开发时会发现的。

Copy link
Member

Choose a reason for hiding this comment

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

如果 pageUrl 是当前站点的,以 / 开头,可以通过判断 this.path 是否等于 pageUrl 来解决的


// notfound handler is unimplemented
if (options.pageUrl && this.path === options.pageUrl) {
this.body = `${htmlTitle}config.notfound.pageUrl(${options.pageUrl}) is unimplemented`;
Copy link
Member Author

Choose a reason for hiding this comment

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

加上了,没有做环境判断的区分,统一返回这个文案了。


// notfound handler is unimplemented
if (options.pageUrl && this.path === options.pageUrl) {
this.body = `${notFoundHtml}config.notfound.pageUrl(${options.pageUrl}) is unimplemented`;
Copy link
Member

@fengmk2 fengmk2 Feb 13, 2017

Choose a reason for hiding this comment

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

加个 p

`${notFoundHtml}
<p><pre><code>config.notfound.pageUrl(${options.pageUrl})</code></pre> is unimplemented</p>`;

@codecov
Copy link

codecov bot commented Feb 13, 2017

Codecov Report

Merging #368 into master will not change coverage.
The diff coverage is 100%.

@@          Coverage Diff          @@
##           master   #368   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          28     28           
  Lines         636    637    +1     
=====================================
+ Hits          636    637    +1
Impacted Files Coverage Δ
config/config.default.js 100% <ø> (ø)
app/middleware/notfound.js 100% <100%> (ø)

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 0bbfa7b...436ebe8. Read the comment docs.

@popomore
Copy link
Member

这个配置好像影响不大,就不记录了

@fengmk2 fengmk2 merged commit 1d93002 into master Feb 13, 2017
@fengmk2 fengmk2 deleted the 404 branch February 13, 2017 11:10
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.

5 participants