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

server.dependency: call after with (server, options, next). Closes #2520 #2574

Closed
wants to merge 1 commit into from

Conversation

garthk
Copy link

@garthk garthk commented May 31, 2015

This fixes #2520 without breaking any other tests, but I'm not entirely happy with it:

  • The arguments list for the Plugin constructor is getting a little long
  • That check for ext.arity in Server.start doesn't explain itself
  • Plugin.after is getting too clever with its arguments

(Update: cleaned up a little in later commits, now squashed in.)

@garthk
Copy link
Author

garthk commented May 31, 2015

Heh. Finding those lint errors was tough; I started with the lint branch, which has ~200 lines of lint complaints. Travis had a shorter list, but with line numbers based on master.

@hueniverse
Copy link
Contributor

Can't check the number of arguments in a callback function like this. It is never a good idea because you don't know how the method is implemented internally. It can have more than 3 arguments or no arguments at all and use arguments.

@garthk
Copy link
Author

garthk commented May 31, 2015

Cleaned up the mess I'd made of _afters by pre-binding the after method's leading arguments. Removed checks against arguments.length as a side-effect. Cost: another private API method.

Any hints on how to rebase to master? I've been using Mercurial for half a decade.

@garthk
Copy link
Author

garthk commented May 31, 2015

Rebased and squashed. Travis failed for io.js because npm install failed. Re-running the build should clear the error.

FWIW, I won't be at all offended if you ditch the PR because it doesn't pass muster. I'm delighted just to be able to file a PR.

@hueniverse hueniverse closed this Aug 12, 2015
@hueniverse hueniverse added the feature New functionality or improvement label Aug 12, 2015
@hueniverse hueniverse self-assigned this Aug 12, 2015
@lock
Copy link

lock bot commented Jan 9, 2020

This thread has been automatically locked due to inactivity. Please open a new issue for related bugs or questions following the new issue template instructions.

@lock lock bot locked as resolved and limited conversation to collaborators Jan 9, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
feature New functionality or improvement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

API wart: server.dependency after method lacks options
2 participants