tomas / tomas/needle

Proposal: Supporting User-Defined "Follow If" Conditions

Open
#407 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

Dominant language
JavaScript
Stars
1.6k
Forks
237
PR merge metrics
No merged PRs in 30d

Description

I needed a bit more control over the follow-if logic today, so I created a small patch to enable the declaration of numerous user-defined conditions:

const options = {
    follow_max: 5,
    follow_if_all: [
        // Only follow if the new endpoint is HTTP or HTTPS
        ( redirect, original ) => /^https?:/.test( redirect )
    ]
};

I'm okay with a redirect from HTTP to HTTPS, or the other way around, but I encountered a URL today that aimed to redirect from HTTPS to MAILTO. This would throw from within http.request, causing all sorts of problems for me.

I'd love to hear everybody's thoughts about this proposal.

Here is the diff that solved my problem:

diff --git a/node_modules/needle/lib/needle.js b/node_modules/needle/lib/needle.js
index a8a1627..509bb37 100644
--- a/node_modules/needle/lib/needle.js
+++ b/node_modules/needle/lib/needle.js
@@ -105,7 +105,10 @@ var defaults = {
   follow_keep_method      : false,
   follow_if_same_host     : false,
   follow_if_same_protocol : false,
-  follow_if_same_location : false
+  follow_if_same_location : false,
+
+  // custom
+  follow_if_all: []
 }
 
 var aliased = {
@@ -258,7 +261,7 @@ Needle.prototype.setup = function(uri, options) {
 
   function check_value(expected, key) {
     var value = get_option(key),
-        type  = typeof value;
+        type  = Array.isArray( value ) ? 'array' : typeof value;
 
     if (type != 'undefined' && type != expected)
       throw new TypeError(type + ' received for ' + key + ', but expected a ' + expected);
@@ -290,6 +293,10 @@ Needle.prototype.setup = function(uri, options) {
     config[key] = check_value('number', key);
   })
 
+  keys_by_type(Array).forEach(function(key) {
+    config[key] = check_value('array', key);
+  })
+
   // populate http_opts with given TLS options
   tls_options.split(' ').forEach(function(key) {
     if (typeof options[key] != 'undefined') {
@@ -511,6 +518,10 @@ Needle.prototype.should_follow = function(location, config, original) {
   if (config.follow_if_same_protocol && !matches('protocol'))
     return false; // procotol does not match, so not following
 
+  if ( config.follow_if_all && !config.follow_if_all.every( o => o( location ) ) ) {
+    return false; // One of the user-provided conditions were not met
+  }
+
   return true;
 }
 

This issue body was partially generated by patch-package.

Contributor guide

No contributing guide indexed for this repository

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start with node_modules/needle/lib/needle.js, especially Needle.prototype.setup and Needle.prototype.should_follow, and review the existing follow_if_* options. Check how redirect handling and option validation are tested; done means the proposed follow_if_all behavior works without breaking existing follow rules and has regression coverage.

Written by the indexing model from the issue text.

Assessment

Tech stack
javascript, node.js
Domain
api
Issue type
Feature
Difficulty
4/5
Estimated time
3-5 days
Activity status
Stale
Clarity
Mostly clear
Newbie friendliness
35/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.