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

allow deep-merging of options? #1180

Open
cowboy opened this issue Jul 15, 2014 · 9 comments
Open

allow deep-merging of options? #1180

cowboy opened this issue Jul 15, 2014 · 9 comments

Comments

@cowboy
Copy link
Member

cowboy commented Jul 15, 2014

Some task authors have expressed a desire for this.options() to (recursively) deep merge task options. Right now, options are shallow merged (one level deep).

(Currently, target-level config options override task-level config options, which override built-in task options. Nobody is asking for this to change, yay)

Why aren't options deep merged? Because options are atomic. While individual options must be merged into a single object, their values should be left unchanged.

To better understand this, we must understand what an option is. An option is a single value used to configure a task. Tasks can be configured with zero or more options. Thus, the merged options object may contain, as its properties, zero or more options, each with a corresponding value.

When one thinks about options in this way, the idea of deep merging all options seems a bit heavy handed. So I'll propose an alternative, although I'm not sure what it should look like yet.

What if task authors could specify, inside the task where they specify the built-in default values, a per-option setting that enables deep merging for that option? The setting could be a number representing the max depth. And it would default to 0 or false for a shallow merge.

Thoughts?

@vladikoff vladikoff added this to the 0.5.0 milestone Jul 15, 2014
@cowboy cowboy modified the milestones: 0.4.6, 0.5.0 Jul 15, 2014
@mzgoddard
Copy link

This definitely sounds useful for complex tasks. A tricky thing I think will be how tricky merging objects deeply is. We can provide a simple mechanism, but it probably won't be complicated enough for some. I think we should provide an interface that can be given a function similar to a comparator. I'm not sure if there is a name for something like this already.

this.optionsMerger(function(key, self, other, merge, userdata) {
  if (typeof other === 'object') {
    return merge(self, other, userdata);
  } else {
    return other;
  }
});
var options = this.options();

An object listing depth could be normalized to this function storing root key and depth in userdata to check if it should merge deeper.

This whole issue seems like it could be rather complex to figure out and also explain whatever is decided but useful.

@sindresorhus
Copy link
Member

What are the use-cases?

I've personally yet to see any need or demand for this though.

@cowboy
Copy link
Member Author

cowboy commented Jul 18, 2014

I don't have any personal use-cases, but I know a discussion was happening around gruntjs/grunt-contrib-concat#59, and there might be others. If this is a common request, perhaps it should be built-in.

@vladikoff
Copy link
Member

The nested options help us keep configuration clean, keep the READMEs clean, etc.
Here are 3 examples of nested options:

Soucemap configuration

options: {
  sourceMap: true
},

or

options: {
  sourceMap: {
    name: 'tmp/sourcemap3_embed_map.map',
    style: ...
  }
}

Watch livereload

    options: {
      livereload: true,
    },

or

options: {
      livereload: {
        port: 9000,
        key: grunt.file.read('path/to/ssl.key'),
        cert: grunt.file.read('path/to/ssl.crt')
      }
    },

Connect

server: {
      options: {
        port: 8000,
        base: {
          path: 'www-root',
          options: {
            index: 'somedoc.html',
            maxAge: 300000
          }
        }
      }

@paazmaya
Copy link

👍

@andrezero
Copy link

+1 This would be great.

As a user of grunt tasks, it feels frustrating that to override a 2nd level property you need to override the whole option. Probably grunt wasn't designed with such complexity in mind, but the fact is many tasks have N deep configuration.

Also, as a grunt task developer I always try to avoid nesting configuration, because of lack of support for deep merge, but if feels weird to expose task with a configuration like this:

examplesBaseUrl: '<%= vars.docs.examplesBaseUrl %>',
examplesScripts: '<%= vars.docs.examplesScripts %>',
examplesStyles: '<%= vars.docs.examplesStyles %>',

In the above case, I have the need to override only the baseUrl per targets.

@stdavis
Copy link

stdavis commented Feb 27, 2015

I would also love deep merging of the options by default.

Could do it on my own using grunt.config(taskName) to get the config object and then deep merging the options on my own? Does this sound like a feasible workaround?

@monolithed
Copy link

monolithed commented Jun 26, 2016

+1, my use case:

Gruntfile

{
    options: {
        hooks: {
            init () {
                fs.readFile('files/logo.txt', {
                    encoding: 'utf-8'
                },
                (error, logo) => {
                    console.log(logo);
                })
            }
        }
    }
}

Actual task

let options = this.options({
    hooks: {
        /*
         * init, 
         * done,
         * fail 
         */
    }
});

let { hooks } = options;

if (hooks.init) {
    hooks.init();
}

Expected task

let options = this.options({
    hooks: {
        init () {},
        fail () {},
        done () {}
    }
});

let { hooks } = options;

hooks.init();

@travispaul
Copy link

travispaul commented Oct 4, 2016

Easy to workaround but without recursively merging, this.options() is completely useless for me. Surprised to see that this issue is over 2 years old.

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

No branches or pull requests

9 participants