pattern for namespacing

[pixelzoom]

This came up in the context of together, where having namespacing is required for deserialization, JS type verification, and documentation of associated JS types.

Come up with a pattern that we like and apply it to existing code (sims and common).

[pixelzoom]

(1) Pattern similar to @jonathanolson repos (scenery, kite, dot,... )

// functonBuilder.js
define( function( require ) {
  'use strict';
  return ( phet.functionBuilder = {} );
} )

// requires a require
var functionBuilder = require( 'FUNCTION_BUILDER/functionBuilder' );

// immediately after constructor
functionBuilder.PatternsModel = PatternsModel;

// wrap inherit, if you don't mind using the expression result of assignment
return functionBuilder.PatternsModel = inherit( Object, PatternsModel, ... );

// or if the above is distasteful...
functionBuilder.PatternsModel = inherit( Object, PatternsModel, ... );
return functionBuilder.PatternsModel;

[pixelzoom]

(2) Create global function phet.namespace for namespacing. Requires duplication of the namespace token (e.g. 'functionBuilder'). Prone to typos in the name.

// after constructor
phet.namespace( 'functionBuilder.PatternsModel', PatternsModel );

// wrap the inherit
return phet.namespace( 'functionBuilder.PatternsModel', inherit( Object, PatternsModel, ... ) );

[pixelzoom]

@jonathanolson and I are leaning towards (1).

[pixelzoom]

To be clear on why I'm leaning towards (1)....

• The require statement will catch typos, you can't accidentally assign to some other global.
• If you add the require statement and forget to do the namespace assignment, lint will complain.
• The namespace assignment is done via an assignment statement - smells like a duck, looks like a duck. (My preference is for the assignment statement immediately following the constructor.)
• The pattern also looks nice in .js files that don't use inherit.

[pixelzoom]

@samreid @jbphet @aaronsamuel137 @jessegreenberg please comment. Propose additional patterns, and vote on your preferred pattern.

[aaronsamuel137]

+1 for pattern (1)

[samreid]

• The require statement will catch typos, you can't accidentally assign to some other global.

Sure you could, there might be a copy paste error like this:

var functionBuilder = require( 'SCENERY_PHET/sceneryPhet' );

• If you add the require statement and forget to do the namespace assignment, lint will complain.

Yes, but if you forget both, you won't see any lint errors. (However, we could add custom ESLint rules to catch this issue).

The namespace assignment is done via an assignment statement - smells like a duck, looks like a duck.

Assignment using = operator cannot check for collisions. What if a sim has two Electron.js files (one in the view and one in the model?)

• The pattern also looks nice in .js files that don't use inherit.

The following

phet.namespace( 'functionBuilder.PatternsModel', PatternsModel );

doesn't require any inherit call, and works for any type.

Why can't we use Tandem.addInstance for this? It seems like it has very similar goals?

[samreid]

I'm also interested in discussing our plan for namespacing files:

1. On a file-by-file as-needed basis
2. Once any file in a repo needs it we namespace everything in the repo
3. We try to instrument everything all at once

If we are using (2) - (3) we may be incentivized to look into ESLint to check that files are namespaced (we may even be able to check the filename somehow?)

[pixelzoom]

I'd prefer to namespace all of my files at once. Will take me a couple of hours, then I won't have to deal with it again.

[pixelzoom]

9/29/15 dev meeting:
• using Tandem doesn't seem to have significant advantages
• would be nice to create a namespace for a repo, error out if you try to attach something to a namespace that doesn't exist (or the wrong namespace?), check typos in names, etc.
• look at using ESLint to catch related errors
• @jonathanolson unsure how he would change scenery/dot/kite to work with function call (pattern 2, #378 (comment)), still leaning towards pattern 1
• @aaronsamuel137 still leaning heavily towards pattern 1
• @pixelzoom still votes for pattern 1
• main advantage of using a function is error checking (useful) or needing to know that a module was loaded (not currently needed)
• calling a function provides protection/flexibility to be able to do additional stuff in the future
• discussed pattern 3 below

Each developer will try out patterns 1, 2, 3 (or propose new patterns) and report back at Tue 10/6/15 developer meeting.

[jonathanolson]

Pattern 3:

// Namespace.js
define( require ) {
  'use strict';

  function Namespace( name ) {
    this._name = name;

    if ( window.phet ) {
      window.phet[ name ] = this;
    }
  }

  return inherit( Object, Namespace, {
    register: function( name, value ) {
      assert && assert( !this[ name ], 'Duplicate ...' );
      this[ name ] = value;

      console.log( this._name + ' ' + name + ' loaded' );
    }
  } );
}

////////////////

// functionBuilder.js
define( require ) {
  'use strict';

  var Namespace = require( '.../Namespace' ); 

  return new Namespace( 'functionBuilder' );
}

////////////////

var functionBuilder = require( 'FUNCTION_BUILDER/functionBuilder' );  <-- add

function SomeThing() {
  ...
}
functionBuilder.register( 'SomeThing', SomeThing ); // pattern 3

functionBuilder.SomeThing = SomeThing; // pattern 1

[pixelzoom]

Unassigning. Will be discussed at 10/6/15 developer meeting after we individually test-drive.

[pixelzoom]

I took pattern (3) for a test-drive, see these (committed) files in function-builder:

Namespace.js - namespace abstraction, would be moved to common code (phet-core?)
functionBuilder.js - sim-specific namespace instance
PatternsModel.js - example of namespace in a file that uses inherit
FBColors.js - example of namespace in a file that does not use inherit, returns an object literal

[samreid]

Thoughts and reflections after testing this out?

[samreid]

The term PatternsModel appears 4 times in PatternsModel.js

[pixelzoom]

I like this pattern - I think it combines the best elements of the other 2. I'm not crazy about duplication of the name of the thing being registered (e.g. PatternsModel). But that's an issue with all patterns that we've considered.

[pixelzoom]

One could eliminate one duplication of the thing being registered (e.g. PatternsModel) if the inherit call could be embedded, like this:

return functionBuilder.register( 'PatternsModel', inherit( Object, PatternsModel, ... ) );

But I don't like the look of this, and it somewhat obfuscates the value of what's being registered.

[samreid]

BRAINSTORMING_EMOTICON_NOT_FOUND:

We could move inherit to Namespace and do something like this (only needs a require statement for functionBuilder, not for inherit):

return functionBuilder.inherit( 'PatternsModel', Object, PatternsModel, { //...

[pixelzoom]

Responsibility for inheritance doesn't belong in Namespace.

[samreid]

Not advocating, just brainstorming still: Namespace could be renamed to ModuleSystem which provides namespacing and inheritance functions (related functions for creating a module).

[pixelzoom]

Namespace could be renamed to ModuleSystem

How is this a module system? AMD is a module system, and is API specification is implemented by requirejs.

If it's indeed a module system, then why do we need requirejs? Or why would we want to have 2 module systems?

[pixelzoom]

And what is the advantage (other than the syntactic convenience described in #378 (comment)) of combining namespace and inheritance responsibilities?

[samreid]

This issue is getting a bit long, I'll create separate issue(s) for creating the eslint rule(s) for checking namespacing. @pixelzoom anything else to do here?

[pixelzoom]

We should probably prioritize adding namespace to common repos, and divide up the work.

[pixelzoom]

Looks like @jonathanolson took care of axon, phet-core, dot, kite, scenery.

Here are the common repos that don't currently have a namespace. Let's put names next to these at developer meeting.

• brand (one brand.js? where to put it?) @samreid
• joist @pixelzoom
• nitroglycerin @pixelzoom
• phetcommon @pixelzoom
• scenery-phet @pixelzoom
• shred @jessegreenberg & @aadish
• sun @jbphet
• tandem (namespace needed?) @samreid
• together (namespace needed? together.js is used for something else)
• vegas @pixelzoom
• vibe @pixelzoom

[pixelzoom]

I've created individual issues (see above) for all of the common repos that need a namespace. After assigning those issues, we can close this issue.

[pixelzoom]

@jbphet says that sun will be done today. Closing.