search

benkroeger / oniyi-http-client

Adding a plugin interface to "request" that allows modifications of request parameters and response data.
MIT License
0 stars 1 forks source link

Feature/add phase skipping #11

Closed futin closed 6 years ago

futin commented 6 years ago

Renamed "helpers" directory into "utils", since ava ignores "helpers" directory by default.

If you approve this idea, I'll update readme.md before merging

futin commented 6 years ago

@benkroeger Can I please have some feedback on this implementation?

benkroeger commented 6 years ago

renaming helpers to utils is fine... embedding the feature to skip a phase is not something I want centralized (as discussed already). If any of your plugins needs skipping, make it a plugin option and manage skipping info across multiple plugins via hookState

futin commented 6 years ago

But then each plugin would have to handle this logic and verify if it is marked for skipping by reading the hookState... which is exactly what I've built(except that each plugin was checking requestOptions.phasesToSkip prop), and you said that it should be changed...

seems like code that is better suited in oniyi-http-client... and there, actual plugin execution should be skipped.

Comment can be found here. Just trying to clear the confusion.

benkroeger commented 6 years ago

so, help me understand this... the current implementation clones a phaseList incl. their handlers (which are functions basically) and then removes those cloned functions from the cloned phaseList. right? Feels like this is a little overhead on making a single http request... particularly memory-wise. Another way would be to not use _.cloneDeep but [].map() and [].filter() instead.

Also, since this seems to be currently only needed for your cache plugin, I'm wondering if it wouldn't in-fact be better to do this inside that one plugin. I don't see how skipping an entire phase inside a phaseList is a huge benefit. this approach doesn't look like it's fine granular enough. Sry for the confusion thoug

futin commented 6 years ago

the current implementation clones a phaseList incl. their handlers (which are functions basically) and then removes those cloned functions from the cloned phaseList. right?

Correct.

Another way would be to not use _.cloneDeep but [].map() and [].filter() instead

I've used _.cloneDeep() in order not to make a shallow copy, keep a .remove() method that removes the handlers(functions) and to be able to use .run() method. But yeah, it can probably be done by removing handlers manually from _phaseMap and _phases.

Also, since this seems to be currently only needed for your cache plugin, I'm wondering if it wouldn't in-fact be better to do this inside that one plugin.

Currently, yes. But we might not need to use template-url plugin for every request as well, and that is why my idea was to provide this validation in every PlaginHookHandler, just in case we someone else requires skipping.

Conclusion:

Keep the old implementation of skipping logic in all plugins? Keep it only within caching plugin and remove it from other plugins?

benkroeger commented 6 years ago

so, the template-url plugin doesn't do anything when there is no template string in the url... no need to add a skip logic there. Keep it only in the caching plugin for now and remove it from the others. we can add this logic back when there is an actual need for it.