search

iron-meteor / iron-middleware-stack

Client and server middleware support inspired by Connect.
MIT License
8 stars 13 forks source link

Remove name=>handler mapping from the stack. #8

Closed nathan-muir closed 8 years ago

nathan-muir commented 8 years ago

https://github.com/iron-meteor/iron-router/issues/1513

I can't find any code in iron-router that even uses any of the name-related API's. (findByName, insertBefore and insertAfter).

But it is causing unusual breakages because names are auto-assigned based on the function name.

dburles commented 8 years ago

+1 thanks @nathan-muir

noosphere2 commented 8 years ago

+1 thanks @nathan-muir

manybothans commented 8 years ago

+1

mikkhait commented 8 years ago

+1, please merge

cmaddalozzo commented 8 years ago

+1, can we get a merge please?

tmeasday commented 8 years ago

Ok, this has now bitten me. I guess I can merge and release this ;)

@nathan-muir I wonder, would it be less of a change to just remove this line: https://github.com/nathan-muir/iron-middleware-stack/blob/remove-names-from-stack/lib/handler.js#L49-L50

That way you can still name handlers, it'll just not "auto-name" them..

dburles commented 8 years ago

That's weird I was literally about to ping you in here when your comment appeared @tmeasday :smile:

nathan-muir commented 8 years ago

@dburles @tmeasday Yeah, that would work too - I just thought, get rid of the unused apis!

tmeasday commented 8 years ago

I mean.. people could be using them, right?

nathan-muir commented 8 years ago

https://github.com/iron-meteor/iron-router/issues/1513

I can't find any code in iron-router that even uses any of the name-related API's. (findByName, insertBefore and insertAfter). So there are really two options:

  1. Add some option / flag / type that bypasses names in MiddlewareStack. (Maintaining existing API's if someone is using it?)
  2. Just remove anything name related from MiddlewareStack, and bump a major version.
tmeasday commented 8 years ago

Am I misunderstanding something though -- like if we literally just remove those two lines of code, it will solve the problem, not change the APIs. Potentially people are relying on the behavior (of naming things from function names) but a) I doubt it b) it'd break in the end anyway, because this issue.

I think we could do that an bump a minor version?

nathan-muir commented 8 years ago

That's definitely the less aggressive option, especially if you think there is anyone using this outside iron:router.

I'm just advocating making the codebase leaner, leaving less places for bugs like this to pop up in future.

nathan-muir commented 8 years ago

But yeah, I don't really mind whatever change gets made. I'm not the maintainer here.

tmeasday commented 8 years ago

Alright, appreciate that @nathan-muir. I think minimal changes are good given I'm pretty unfamiliar with the code base these days.