.isView() heuristic is missing "content" property

[mootari]

Unless I'm missing something here I would assume that "content" should be checked as well.

The implementation in assemble-loader:

assemble-loader/utils.js

Lines 38 to 40 in 6d6b001

	utils.isView = function(obj) {
	return utils.isObject(obj) && (obj.path || obj.contents || obj.isView || obj.isItem);
	};

For comparison, the implementation in templates:
https://github.com/jonschlinkert/templates/blob/f030d2c2906ea9a703868f83574151badb427573/lib/utils.js#L319-L323

[assemblebot]

@mootari Thanks for the issue! If you're reporting a bug, please be sure to include:

• The version of assemble you are using.
• Your assemblefile.js (This can be in a gist)
• The commandline output. (Screenshot or gist is fine)
• What you expected to happen instead.

[jonschlinkert]

The check here might make more sense. If .contents exists, then .content should exist as well since the properties are synced. If .contents does not exist, then it seems like we wouldn't want to check for .content, since the existence of only one of those properties indicates it's not a valid View. thoughts? @doowb?

Some of this logic was done as we were still building the API and learning about what we were creating. Thanks for creating issues, it's much appreciated.

[mootari]

Not sure if this is relevant, but utils.normalizeContent() in load-templates checks for both content and contents:
https://github.com/jonschlinkert/load-templates/blob/995ccaf1955652ad3401d8457f331917b308b110/utils.js#L36-L47

[doowb]

This specific isView method is only used in this library to determine if a passed in object looks like a view or an options object. Another piece of code handles normalizing and "syncing" the content and contents properties later so I think that we should add a check for .content in this specific method.

@mootari if you'd like to do a PR to update this piece of code, that would be awesome! I think you pointed out in another issue that some of these use hasOwnProperty and some don't. I think this should be updated to either using .hasOwnProperty or typeof obj.property === 'undefined'.

Also, if you have ideas on consolidating the places where this code is duplicated into a single module that can be reused that would be great! Thanks for getting into the code and opening these issues, it really helps.

[jonschlinkert]

think this should be updated to either using .hasOwnProperty or typeof obj.property === 'undefined'.

We don't want to use typeof obj.property === 'undefined' since it's possible for a property to be defined with an undefined value. I believe .contents and .content are both enumerable so .hasOwnProperty() should be find. Otherwise we should use !(key in view).

[mootari]

I did a quick search through assemble's dependencies. The following places call .isView():

1. assemble-loader/index.js

   Line 132 in 6d6b001

   if (utils.isView(options)) options = {};

2. https://github.com/jonschlinkert/load-templates/blob/995ccaf1955652ad3401d8457f331917b308b110/index.js#L60
3. https://github.com/jonschlinkert/load-templates/blob/995ccaf1955652ad3401d8457f331917b308b110/index.js#L69
4. https://github.com/jonschlinkert/load-templates/blob/995ccaf1955652ad3401d8457f331917b308b110/index.js#L180
5. https://github.com/jonschlinkert/load-templates/blob/995ccaf1955652ad3401d8457f331917b308b110/index.js#L198
6. https://github.com/jonschlinkert/templates/blob/f030d2c2906ea9a703868f83574151badb427573/lib/views.js#L259

[mootari]

1. Looks like overloading for the options param, but the intention is not clear to me.
2. For an array of templates, used to delegate each to either .addView() and .load()
3. Again, looks like overloading for the options param (gate to either .addView() or .globViews())
4. Overloads for the options param.
5. Initializes path and key prior to passing off to .addView()
6. Optional view argument, not clear where/when used.