search

brunch / jshint-brunch

Adds JSHint linting support to Brunch.
7 stars 4 forks source link

clone nested objects from config #7

Closed es128 closed 11 years ago

es128 commented 11 years ago

in case of freeze issues

es128 commented 11 years ago

related to https://github.com/brunch/uglify-js-brunch/issues/7

paulmillr commented 11 years ago

@es128 maybe we can come up with more general solution for that? perhaps in brunch?

so, why does current behavior (errors) happen? we don’t mutate cfg in place

es128 commented 11 years ago

Honestly, I'm not sure if it's a problem here. You asked me to check for similar situations after the issue with uglify and this was the only similar one I found (+coffeelint).

In the uglify plugin, I'm pretty sure the error was being thrown from within uglify itself, so its internal code must have been trying to write to the config object that got passed in. I don't know if the same thing might come up in the linters, but I guess the point is that we need to be careful since we don't have that much control over what happens in the components.

As for a more general solution, here are some ideas (some could be combined):

  1. clone config in brunch before passing it into any plugins, although that begins to defeat the purpose of freeze. Plus, unless you make a separate clone for each plugin then they can impact each other.
  2. pass only the plugin.{name} object into each plugin for it's config, but that would be a big breaking change. The plugin keys would have be standardized somehow, or each plugin class could specify its key name.
  3. I guess the presence of configKey in the plugin class could also be like a flag to switch to a backward-compat behavior.
  4. Don't freeze the plugins object, or clone each nested object in plugins separately, could even reconstruct the config.plugins[key] structure in each copy for backward compatibility.

I suppose my overall suggestion would be for brunch to start looking for configKey, and if present, clone the corresponding nested object to pass into the plugin.

Do plugins ever care about the overall brunch config object outside of their own settings? I guess just in case, the frozen config object could also be passed.

paulmillr commented 11 years ago

@es128 right, freeze is done with purpose. You shouldn’t mutate config anywhere. If you do, then clone. I guess uglify did this internally so it was shitty. So we don’t need this change here I think.

All config is passed “just in case”. I think some plugins (bower-brunch) incorporate it.

es128 commented 11 years ago

Sounds good. We'll just keep it in mind in case a similar issue ever comes up again.