Skip to content

Commit

Permalink
Updates based on code review.
Browse files Browse the repository at this point in the history
  • Loading branch information
@kevinpschaaf
kevinpschaaf committed Jul 21, 2018
1 parent a515c99 commit 36c4dfa
Show file tree
Hide file tree
Showing 7 changed files with 190 additions and 48 deletions.
4 changes: 3 additions & 1 deletion lib/elements/dom-if.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -251,7 +251,9 @@ export class DomIf extends PolymerElement {
if (c$ && c$.length) {
// use first child parent, for case when dom-if may have been detached
let parent = c$[0].parentNode;
if (parent) {
// Instance children may be disconnected from parents when dom-if
// detaches if a tree was innerHTML'ed
if (parent) {
for (let i=0, n; (i<c$.length) && (n=c$[i]); i++) {
parent.removeChild(n);
}
Expand Down
20 changes: 12 additions & 8 deletions lib/elements/dom-module.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,12 @@ import { strictTemplatePolicy } from '../utils/settings.js';

let modules = {};
let lcModules = {};
function setModule(id, module) {
// store id separate from lowercased id so that
// in all cases mixedCase id will stored distinctly
// and lowercase version is a fallback
modules[id] = lcModules[id.toLowerCase()] = module;
}
function findModule(id) {
return modules[id] || lcModules[id.toLowerCase()];
}
Expand Down Expand Up @@ -122,16 +128,14 @@ export class DomModule extends HTMLElement {
register(id) {
id = id || this.id;
if (id) {
if (strictTemplatePolicy && findModule(id)) {
modules[id] = lcModules[id.toLowerCase()] = null;
throw new Error(`strictTemplatePolicy: dom-module ${id} registered twice`);
// Under strictTemplatePolicy, reject and null out any re-registered
// dom-module since it is ambiguous whether first-in or last-in is trusted
if (strictTemplatePolicy && findModule(id) !== undefined) {
setModule(id, null);
throw new Error(`strictTemplatePolicy: dom-module ${id} re-registered`);
}
this.id = id;
// store id separate from lowercased id so that
// in all cases mixedCase id will stored distinctly
// and lowercase version is a fallback
modules[id] = this;
lcModules[id.toLowerCase()] = this;
setModule(id, this);
styleOutsideTemplateCheck(this);
}
}
Expand Down
23 changes: 6 additions & 17 deletions lib/legacy/class.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -150,22 +150,6 @@ function GenerateClassFromInfo(info, Base) {
return info.observers;
}

/**
* @return {HTMLTemplateElement} template for this class
*/
static get template() {
// get template first from any imperative set in `info._template`
return info._template ||
// next look in dom-module associated with this element's is.
(!strictTemplatePolicy && (DomModule && DomModule.import(this.is, 'template'))) ||
// next look for superclass template (note: use superclass symbol
// to ensure correct `this.is`)
Base.template ||
// finally fall back to `_template` in element's prototype.
this.prototype._template ||
null;
}

/**
* @return {void}
*/
Expand Down Expand Up @@ -363,5 +347,10 @@ export const Class = function(info) {
LegacyElementMixin(HTMLElement));
// decorate klass with registration info
klass.is = info.is;
return klass;
// if user provided template on info, make sure the static _template
// is set so the static template getter uses it
if (info._template !== undefined) {
klass._template = info._template;
}
return klass;
};
42 changes: 33 additions & 9 deletions lib/mixins/element-mixin.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ import { pathFromUrl, resolveCss, resolveUrl as resolveUrl$0 } from '../utils/re
import { DomModule } from '../elements/dom-module.js';
import { PropertyEffects } from './property-effects.js';
import { PropertiesMixin } from './properties-mixin.js';
import { strictTemplatePolicy } from '../utils/settings.js';
import { strictTemplatePolicy, allowTemplateFromDomModule } from '../utils/settings.js';

/**
* Element class mixin that provides the core API for Polymer's meta-programming
Expand Down Expand Up @@ -270,6 +270,27 @@ export const ElementMixin = dedupingMixin(base => {
}
}

/**
* Look up template from dom-module for element
*
* @param {!string} is Element name to look up
* @return {!HTMLTemplateElement} Template found in dom module, or
* undefined if not found
* @private
*/
function getTemplateFromDomModule(is) {
let template = null;
if (is && allowTemplateFromDomModule) {
template = DomModule.import(is, 'template')
// Under strictTemplatePolicy, require any element with an `is`
// specified to have a dom-module
if (strictTemplatePolicy && !template) {
throw new Error(`strictTemplatePolicy: expecting dom-module or null template for ${is}`);
}
}
return template;
}

/**
* @polymer
* @mixinClass
Expand Down Expand Up @@ -379,15 +400,18 @@ export const ElementMixin = dedupingMixin(base => {
*/
static get template() {
if (!this.hasOwnProperty(JSCompiler_renameProperty('_template', this))) {
this._template = (!strictTemplatePolicy && DomModule && DomModule.import(
/** @type {PolymerElementConstructor}*/ (this).is, 'template')) ||
// note: implemented so a subclass can retrieve the super
// template; call the super impl this way so that `this` points
// to the superclass.
Object.getPrototypeOf(/** @type {PolymerElementConstructor}*/ (this).prototype).constructor.template;
this._template =
// Look in dom-module associated with this element's is
getTemplateFromDomModule(/** @type {PolymerElementConstructor}*/ (this).is) ||
// Next look for superclass template (call the super impl this
// way so that `this` points to the superclass)
Object.getPrototypeOf(/** @type {PolymerElementConstructor}*/ (this).prototype).constructor.template ||
// Finally, fall back to any _template set on prototype, e.g.
// via registered callback
this.prototype._template;
}
return this._template;
}
}

/**
* Path matching the url from which the element was imported.
Expand All @@ -414,7 +438,7 @@ export const ElementMixin = dedupingMixin(base => {
if (meta) {
this._importPath = pathFromUrl(meta.url);
} else {
const module = DomModule && DomModule.import(/** @type {PolymerElementConstructor} */ (this).is);
const module = DomModule.import(/** @type {PolymerElementConstructor} */ (this).is);
this._importPath = (module && module.assetpath) ||
Object.getPrototypeOf(/** @type {PolymerElementConstructor}*/ (this).prototype).constructor.importPath;
}
Expand Down
23 changes: 21 additions & 2 deletions lib/utils/settings.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -86,8 +86,8 @@ export const setPassiveTouchGestures = function(usePassive) {

/**
* Setting to ensure Polymer template evaluation only occurs based on tempates
* defined in trusted script. When true, `<dom-module>` based template lookup
* is disabled, `<dom-bind>` is disabled, and `<dom-if>`/`<dom-repeat>`
* defined in trusted script. When true, `<dom-module>` re-registration is
* disallowed, `<dom-bind>` is disabled, and `<dom-if>`/`<dom-repeat>`
* templates will only evaluate in the context of a trusted element template.
*/
export let strictTemplatePolicy = false;
Expand All @@ -102,3 +102,22 @@ export let strictTemplatePolicy = false;
export const setStrictTemplatePolicy = function(useStrictPolicy) {
strictTemplatePolicy = useStrictPolicy;
};

/**
* Setting to enable dom-module lookup from Polymer.Element. By default,
* templates must be defined in script using the `static get template()`
* getter and the `html` tag function. To enable legacy loading of templates
* via dom-module, set this flag to true.
*/
export let allowTemplateFromDomModule = false;

/**
* Sets `lookupTemplateFromDomModule` globally for all elements
*
* @param {boolean} allowDomModule enable or disable template lookup
* globally
* @return {void}
*/
export const setAllowTemplateFromDomModule = function(allowDomModule) {
allowTemplateFromDomModule = allowDomModule;
};
3 changes: 3 additions & 0 deletions lib/utils/templatize.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -496,6 +496,9 @@ and this string can then be deleted`;
* @suppress {invalidCasts}
*/
export function templatize(template, owner, options) {
// Under strictTemplatePolicy, the templatized element must be owned
// by a (trusted) Polymer element, indicated by existence of _methodHost;
// e.g. for dom-if & dom-repeat in main document, _methodHost is null
if (strictTemplatePolicy && !owner._methodHost) {
throw new Error('strictTemplatePolicy: template owner not trusted');
}
Expand Down
123 changes: 112 additions & 11 deletions test/unit/strict-template-policy.html
View file
Original file line number Diff line number Diff line change
Expand Up @@ -37,34 +37,57 @@
</head>
<body>

<dom-module id="trusted-element">
<dom-module id="no-dm">
<template>Should never be used</template>
<script type="module">
import {PolymerElement} from '../../polymer-element.js';
class TrustedElement extends PolymerElement {
static get is() { return 'trusted-element'; }
static get is() { return 'no-dm'; }
}
customElements.define(TrustedElement.is, TrustedElement);
</script>
</dom-module>

<dom-module id="trusted-element-legacy">
<dom-module id="no-dm-legacy">
<template>Should never be used</template>
<script type="module">
import {Polymer} from '../../polymer-legacy.js';
Polymer({is: 'trusted-element-legacy'});
Polymer({is: 'no-dm-legacy'});
</script>
</dom-module>

<dom-module id="trusted-element">
<template>Trusted</template>
<script type="module">
import {PolymerElement} from '../../polymer-element.js';
class TrustedElement extends PolymerElement {
static get is() { return 'trusted-element'; }
}
customElements.define(TrustedElement.is, TrustedElement);
</script>
</dom-module>

<dom-module id="trusted-element-legacy">
<template>Trusted</template>
<script type="module">
import {Polymer} from '../../polymer-legacy.js';
Polymer({is: 'trusted-element-legacy'});
</script>
</dom-module>

<div id="target"></div>

<script type="module">
import {PolymerElement} from '../../polymer-element.js';
import {Polymer} from '../../polymer-legacy.js';
import {flush} from '../../lib/utils/flush.js';
import {setAllowTemplateFromDomModule} from '../../lib/utils/settings.js';

suite('strictTemplatePolicy', function() {

teardown(function() {
window.uncaughtErrorFilter = window.top.uncaughtErrorFilter = null;
document.getElementById('target').textContent = '';
});

// Errors thrown in custom element reactions are not thrown up
Expand All @@ -76,7 +99,9 @@
// Safari errors are thrown on the top window, sometimes not, so
// catch in both places
window.uncaughtErrorFilter = window.top.uncaughtErrorFilter = function(err) {
uncaughtError = err;
if (!uncaughtError) {
uncaughtError = err;
}
return true;
};
assert.throws(function() {
Expand Down Expand Up @@ -131,28 +156,104 @@
});

test('dom-module never used', function() {
var el = document.createElement('trusted-element');
var el = document.createElement('no-dm');
document.getElementById('target').appendChild(el);
assert.notOk(el.shadowRoot);
});

test('dom-module never used (legacy)', function() {
var el = document.createElement('trusted-element-legacy');
var el = document.createElement('no-dm-legacy');
document.getElementById('target').appendChild(el);
assert.notOk(el.shadowRoot);
});

test('dom-module re-registration throws', function() {
// From this point down, dom-module lookup is globally enabled
test('setAllowTemplateFromDomModule', function() {
setAllowTemplateFromDomModule(true);
});

test('dom-module after registration', function() {
assertThrows(function() {
document.getElementById('target').innerHTML =
'<dom-module id="trusted-element">' +
' <template>' +
' <div id="injected"></div>'+
' </template>`' +
'</dom-module>';
}, /trusted-element registered twice/);
'</dom-module>' +
'<trusted-element></trusted-element>';
}, /trusted-element re-registered/);
const el = document.querySelector('trusted-element');
assert.notOk(el && el.shadowRoot && el.shadowRoot.querySelector('#injected'));
});

test('dom-module before registration', function() {
document.getElementById('target').innerHTML =
'<dom-module id="has-no-template">' +
' <template>' +
' <div id="injected"></div>'+
' </template>`' +
'</dom-module>';
class HasNoTemplate extends PolymerElement {
static get is() { return 'has-no-template'; }
static get template() { return null; }
}
customElements.define(HasNoTemplate.is, HasNoTemplate);
let el = document.createElement('has-no-template');
document.getElementById('target').appendChild(el);
assert.notOk(el.shadowRoot);
});

test('dom-module after registration (legacy)', function() {
assertThrows(function() {
document.getElementById('target').innerHTML =
'<dom-module id="trusted-element-legacy">' +
' <template>' +
' <div id="injected"></div>'+
' </template>`' +
'</dom-module>' +
'<trusted-element-legacy></trusted-element-legacy>';
}, /trusted-element-legacy re-registered/);
const el = document.querySelector('trusted-element-legacy');
assert.notOk(el && el.shadowRoot && el.shadowRoot.querySelector('#injected'));
});

test('dom-module before registration (legacy)', function() {
document.getElementById('target').innerHTML =
'<dom-module id="has-no-template-legacy">' +
' <template>' +
' <div id="injected"></div>'+
' </template>`' +
'</dom-module>';
Polymer({
is: 'has-no-template-legacy',
_template: null
});
let el = document.createElement('has-no-template-legacy');
document.getElementById('target').appendChild(el);
assert.notOk(document.querySelector('has-no-template-legacy').shadowRoot);
});

test('element without explicit template throws', function() {
assertThrows(function() {
class HasNoTemplateThrows extends PolymerElement {
static get is() { return 'has-no-template-throws'; }
}
customElements.define(HasNoTemplateThrows.is, HasNoTemplateThrows);
var el = document.createElement('has-no-template-throws');
document.getElementById('target').appendChild(el);
}, /expecting dom-module or null template/);
});

test('element without explicit template throws (legacy)', function() {
assertThrows(function() {
Polymer({
is: 'has-no-template-throws-legacy'
});
var el = document.createElement('has-no-template-throws-legacy');
document.getElementById('target').appendChild(el);
}, /expecting dom-module or null template/);
});

});
</script>

Expand Down

0 comments on commit 36c4dfa

Please sign in to comment.