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: remove overrideMethod middleware #324

Merged
merged 1 commit into from
Feb 7, 2017
Merged

Conversation

fengmk2
Copy link
Member

@fengmk2 fengmk2 commented Feb 7, 2017

It's not safe it enable by default in egg

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

@mention-bot
Copy link

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

@@ -6,10 +6,10 @@ const MAX_AGE = 'public, max-age=2592000'; // 30 days
module.exports = options => {
return function* siteFile(next) {
if (this.method !== 'HEAD' && this.method !== 'GET') return yield next;

if (!options.hasOwnProperty(this.path)) return yield next;
if (this.path[0] !== '/') return yield next;
Copy link
Member Author

Choose a reason for hiding this comment

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

加个字符串判断,就不担心 hasOwnProperty 被入注了

@popomore
Copy link
Member

popomore commented Feb 7, 2017

这个也是不兼容更新,内部要更新下。还有为啥去掉要写一下

@fengmk2
Copy link
Member Author

fengmk2 commented Feb 7, 2017

@popomore 在内部 egg issue 回复了

@fengmk2 fengmk2 force-pushed the remove-override-method branch 2 times, most recently from b65e6ad to c1759af Compare February 7, 2017 12:47
It's not safe it enable by default in egg
@codecov
Copy link

codecov bot commented Feb 7, 2017

Codecov Report

❗ No coverage uploaded for pull request base (master@0ee4451). Click here to learn what that means.

@@            Coverage Diff            @@
##             master     #324   +/-   ##
=========================================
  Coverage          ?   98.08%           
=========================================
  Files             ?       31           
  Lines             ?      834           
  Branches          ?        0           
=========================================
  Hits              ?      818           
  Misses            ?       16           
  Partials          ?        0
Impacted Files Coverage Δ
app/middleware/notfound.js 100% <ø> (ø)
config/config.default.js 100% <ø> (ø)
app/middleware/site_file.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 0ee4451...e7aa468. Read the comment docs.

@fengmk2
Copy link
Member Author

fengmk2 commented Feb 7, 2017

@popomore 这个没问题就合并发个版本。

@popomore popomore merged commit 08bc84e into master Feb 7, 2017
@popomore popomore deleted the remove-override-method branch February 7, 2017 14:04
@popomore
Copy link
Member

popomore commented Feb 7, 2017

fengmk2 added a commit that referenced this pull request Jun 19, 2017
only allow override method on POST request by default

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

Successfully merging this pull request may close these issues.

4 participants