Extending the Revealing Module Pattern – Why is this a bad idea?

http://invalid-arg.github.io/2013/04/13/extending-the-revealing-module-pattern.html

by invalid_arg • 13 years ago

15 14 13 years ago

invalid-arg.github.io

jonny_eh• 13 years ago

I don't see why this is "bad" other than depending on jQuery to do something not directly related the browser. That could easily be fixed by swapping jQuery out for Underscore, since it would then work in browser and in Node.

tantalor• 13 years ago

The revealing module pattern is an antipattern. Instead, just document which of your methods are private or protected,

  /** @private */
  MyClass.prototype.foo_ = function() {}

mistercow2• 13 years ago

I have to agree with you, if nothing else because existing debugging tools just aren't very good at dealing with the privacy-through-closures pattern. In many cases, it's essentially impossible to inspect the state of a variable that is made private this way, and it's not clear how you would even design a debugger that gave a reasonable interface for doing so.

ratbeard2• 13 years ago

Its annoying, but certainly possible to inspect the state of a local variable inside of a closure. Set a breakpoint inside of one of the exported functions, call it, and you have access to all the local variables within the closure.

I do dislike this pattern as I find the code is less structured, harder to debug, and less performant than attaching functions on the prototype. This outweighs the benefits of encapsulation to me.

geoffschmidt• 13 years ago

I agree that this is an antipattern. We don't use it in Meteor. We did use it in a project that some of us wrote together several years before Meteor -- it was our first big JS project and we were heavily influenced by Crockford's _JavaScript: The Good Parts._, which appealed to our Lispy backgrounds. Our experience was that the added hassle in debugging wasn't worth the dubious benefit of locking rogue team members out of your private methods. And as this article notes, you have to go to significant lengths to preserve basic OOP conveniences such as `instanceof` and `this`.

If you're concerned about coworkers calling your private methods, the right tool is coding standards and code review. If you can't trust your coworkers to use good judgement even in the face of those things, you have bigger problem :) On the other hand, if your goal in using this pattern was to protect your private methods from bad people on the internet, you probably need to rethink your assumption that it is possible to sandbox JavaScript without interpreter-level support.

We do use (function () { ... })() in specific situations when we need to create a new scope, but this would typically only occur in framework code rather than application code (especially once the `linker` branch lands, which lets you declare specific exports and imports at the package level, and then the `meteor` asset build tool wires them up for you so that each package gets just the global environment it asked for.)

Also for now, we prefer to use prototypal inheritance directly rather than use a class library. On the latter, our perspective is that if Meteor developers want to use a class library, they should be able to pick their favorite rather than be stuck with ours.

FuzzyDunlop• 13 years ago

I'm not convinced this is a good idea in JS, where the comment will be completely inaccurate. I'm not exactly sure how you'd differentiate something you decided was protected from something else you decided was private.

This isn't to say that having documentation on what you consider to be the API is a bad idea though. Omitting the functions you don't want or expect people to use is good for public facing docs.

doomslice2• 13 years ago

I don't really see the need for the extend at all. In fact if you don't use it, it will work the same as example 1 and you'll still have access to self.

Or you can just do this.foo = foo; this.value = value; at the end instead of returning an Object.

platz• 13 years ago

Yes extend is unnecessary in this case. The key was saving a reference to self.

RyanZAG• 13 years ago

The pattern also seems like it would be an impediment to performance. The JIT would have to do a lot more work to optimize the function, as well as parse code that isn't really required. So even if the end-result machine code after JIT was performed was the same, the compiler itself would have a much harder time getting there. This can be an issue on mobile devices that are CPU constrained where you want the page to load as fast as possible.

emehrkay• 13 years ago

The problem is the foo function not being apart of the returned object. Something like this is a very easy solution without the need to use call/apply/jquery.extend

http://jsfiddle.net/ABD3C/

gibbitz• 13 years ago

I do understand the difficulty of testing private code in tools like jasmine when you use private variables based on publicly unreachable scopes. When I have to do this, I add a debug object with references to the code. Generally, this code should affect publicly accessible methods and those are more important to test. If they fail, decent in-browser tools like breakpoints and watch lists should reveal the error.

I'm not completely ready to call this an anti-pattern. There's no reason to expose local methods of an Object to the parent scope if they aren't needed. I'm sure that in some cases this could cause the memory footprint of an Object to be larger than desired by preventing garbage collection. Anyway, I would have factored your code more like this:

  var PseudoClass = function(_args){
      // guarantee that "this" is what we expect
      if( this === window ){ // use global for non-browser    implementations
         return new PseudoClass(_args);
      }
      // private variables are local
      var private = function() {;
             this.value = 2; 
          };
      // public variables are on prototype 
      this.constructor.prototype.value = 1; // I'd prolly use a local var and a getter for this
      this.constructor.prototype.public = function(){
          private.call(this); // force scope for private methods
      };
      // testing access (only added when necessary)
      this.constructor.prototype.privateMembers = {
          "private":function(){ private.call(this) }
      };
    };

There's no "self" here, but there are two pieces of insurance around scope. First is the condition that forces the "new" keyword. (This could be written to test against instanceof the function name, but generally problems occur when users forget new, not when they force the scope to be wrong. Last I checked, this is how jQuery tests it too.) Second is calling the private methods with the call() method of the function, allowing us to keep the scope from defaulting to global/window. Sure, you are forced to put these in to avoid errors, but this is programming. If you would like it to throw an error when this === window within private(), just add it. I didn't because a) I'm lazy and b) once the PseudoClass private methods are all called, I would end up removing the un-thrown errors and the privateMembers property before compressing for staging/production to keep size down.

Using a variable like "self" is quite clear in this context and I've seen it done thousands of times. I just wanted to show that using "this" is still possible.

Javascript is a funny language. I often find myself wondering if this was the intended structure of the language as it takes advantage of many of the key elements (closure scopes & prototype) while abstractly appearing like simple Java (or C#). Sure in a perfect world, the wrangling of "this" wouldn't need to be done. I'd say the unpredictable value of "this" in different contexts is the biggest fail of Javascript. I programmed ActionScript (including AS1) for many years and this (pun intended) is the biggest mindf*ck I've come across when regarding them in comparison (considering they're both based on ECMA script). Hope there was something helpful to you in this.

Extending the Revealing Module Pattern – Why is this a bad idea?

Related Stories