From 6f3157fe6debbf6847efede6420987ee8889b263 Mon Sep 17 00:00:00 2001 From: Kara Erickson Date: Wed, 20 May 2020 11:05:46 -0700 Subject: [PATCH] Revert "fix(elements): capture input properties set before upgrading the element (#36114)" This reverts commit 1c8f1799d494254074e71c460db47dd65f58575a because it is causing the side effects test to break on the 9.1.x branch. --- .../elements/src/create-custom-element.ts | 53 ++-------- .../test/create-custom-element_spec.ts | 98 +------------------ 2 files changed, 13 insertions(+), 138 deletions(-) diff --git a/packages/elements/src/create-custom-element.ts b/packages/elements/src/create-custom-element.ts index 2e1069840f..7c2e222cd5 100644 --- a/packages/elements/src/create-custom-element.ts +++ b/packages/elements/src/create-custom-element.ts @@ -145,32 +145,9 @@ export function createCustomElement

( // TODO(andrewseguin): Add e2e tests that cover cases where the constructor isn't called. For // now this is tested using a Google internal test suite. if (this._ngElementStrategy === null) { - const strategy = this._ngElementStrategy = strategyFactory.create(this.injector); - - // Collect pre-existing values on the element to re-apply through the strategy. - const preExistingValues = - inputs.filter(({propName}) => this.hasOwnProperty(propName)).map(({propName}): [ - string, any - ] => [propName, (this as any)[propName]]); - - // In some browsers (e.g. IE10), `Object.setPrototypeOf()` (which is required by some Custom - // Elements polyfills) is not defined and is thus polyfilled in a way that does not preserve - // the prototype chain. In such cases, `this` will not be an instance of `NgElementImpl` and - // thus not have the component input getters/setters defined on `NgElementImpl.prototype`. - if (!(this instanceof NgElementImpl)) { - // Add getters and setters to the instance itself for each property input. - defineInputGettersSetters(inputs, this); - } else { - // Delete the property from the instance, so that it can go through the getters/setters - // set on `NgElementImpl.prototype`. - preExistingValues.forEach(([propName]) => delete (this as any)[propName]); - } - - // Re-apply pre-existing values through the strategy. - preExistingValues.forEach(([propName, value]) => strategy.setInputValue(propName, value)); + this._ngElementStrategy = strategyFactory.create(this.injector); } - - return this._ngElementStrategy!; + return this._ngElementStrategy; } private readonly injector: Injector; @@ -216,26 +193,16 @@ export function createCustomElement

( // Update the property descriptor of `NgElementImpl#ngElementStrategy` to make it enumerable. Object.defineProperty(NgElementImpl.prototype, 'ngElementStrategy', {enumerable: true}); - // Add getters and setters to the prototype for each property input. - defineInputGettersSetters(inputs, NgElementImpl.prototype); - - return (NgElementImpl as any) as NgElementConstructor

; -} - -// Helpers -function defineInputGettersSetters( - inputs: {propName: string, templateName: string}[], target: object): void { - // Add getters and setters for each property input. - inputs.forEach(({propName}) => { - Object.defineProperty(target, propName, { - get(): any { - return this.ngElementStrategy.getInputValue(propName); - }, - set(newValue: any): void { - this.ngElementStrategy.setInputValue(propName, newValue); - }, + // Add getters and setters to the prototype for each property input. If the config does not + // contain property inputs, use all inputs by default. + inputs.map(({propName}) => propName).forEach(property => { + Object.defineProperty(NgElementImpl.prototype, property, { + get: function() { return this.ngElementStrategy.getInputValue(property); }, + set: function(newValue: any) { this.ngElementStrategy.setInputValue(property, newValue); }, configurable: true, enumerable: true, }); }); + + return (NgElementImpl as any) as NgElementConstructor

; } diff --git a/packages/elements/test/create-custom-element_spec.ts b/packages/elements/test/create-custom-element_spec.ts index 3551084d72..7df5327569 100644 --- a/packages/elements/test/create-custom-element_spec.ts +++ b/packages/elements/test/create-custom-element_spec.ts @@ -22,16 +22,12 @@ type WithFooBar = { if (browserDetection.supportsCustomElements) { describe('createCustomElement', () => { - let selectorUid = 0; - let testContainer: HTMLDivElement; let NgElementCtor: NgElementConstructor; let strategy: TestStrategy; let strategyFactory: TestStrategyFactory; let injector: Injector; beforeAll(done => { - testContainer = document.createElement('div'); - document.body.appendChild(testContainer); destroyPlatform(); platformBrowserDynamic() .bootstrapModule(TestModule) @@ -40,23 +36,18 @@ if (browserDetection.supportsCustomElements) { strategyFactory = new TestStrategyFactory(); strategy = strategyFactory.testStrategy; - const {selector, ElementCtor} = createTestCustomElement(); - NgElementCtor = ElementCtor; + NgElementCtor = createCustomElement(TestComponent, {injector, strategyFactory}); // The `@webcomponents/custom-elements/src/native-shim.js` polyfill allows us to create // new instances of the NgElement which extends HTMLElement, as long as we define it. - customElements.define(selector, NgElementCtor); + customElements.define('test-element', NgElementCtor); }) .then(done, done.fail); }); afterEach(() => strategy.reset()); - afterAll(() => { - destroyPlatform(); - document.body.removeChild(testContainer); - (testContainer as any) = null; - }); + afterAll(() => destroyPlatform()); it('should use a default strategy for converting component inputs', () => { expect(NgElementCtor.observedAttributes).toEqual(['foo-foo', 'barbar']); @@ -122,90 +113,7 @@ if (browserDetection.supportsCustomElements) { expect(strategy.inputs.get('barBar')).toBe('barBar-value'); }); - it('should capture properties set before upgrading the element', () => { - // Create a regular element and set properties on it. - const {selector, ElementCtor} = createTestCustomElement(); - const element = Object.assign(document.createElement(selector), { - fooFoo: 'foo-prop-value', - barBar: 'bar-prop-value', - }); - expect(element.fooFoo).toBe('foo-prop-value'); - expect(element.barBar).toBe('bar-prop-value'); - - // Upgrade the element to a Custom Element and insert it into the DOM. - customElements.define(selector, ElementCtor); - testContainer.appendChild(element); - expect(element.fooFoo).toBe('foo-prop-value'); - expect(element.barBar).toBe('bar-prop-value'); - - expect(strategy.inputs.get('fooFoo')).toBe('foo-prop-value'); - expect(strategy.inputs.get('barBar')).toBe('bar-prop-value'); - }); - - it('should capture properties set after upgrading the element but before inserting it into the DOM', - () => { - // Create a regular element and set properties on it. - const {selector, ElementCtor} = createTestCustomElement(); - const element = Object.assign(document.createElement(selector), { - fooFoo: 'foo-prop-value', - barBar: 'bar-prop-value', - }); - expect(element.fooFoo).toBe('foo-prop-value'); - expect(element.barBar).toBe('bar-prop-value'); - - // Upgrade the element to a Custom Element (without inserting it into the DOM) and update a - // property. - customElements.define(selector, ElementCtor); - customElements.upgrade(element); - element.barBar = 'bar-prop-value-2'; - expect(element.fooFoo).toBe('foo-prop-value'); - expect(element.barBar).toBe('bar-prop-value-2'); - - // Insert the element into the DOM. - testContainer.appendChild(element); - expect(element.fooFoo).toBe('foo-prop-value'); - expect(element.barBar).toBe('bar-prop-value-2'); - - expect(strategy.inputs.get('fooFoo')).toBe('foo-prop-value'); - expect(strategy.inputs.get('barBar')).toBe('bar-prop-value-2'); - }); - - it('should allow overwriting properties with attributes after upgrading the element but before inserting it into the DOM', - () => { - // Create a regular element and set properties on it. - const {selector, ElementCtor} = createTestCustomElement(); - const element = Object.assign(document.createElement(selector), { - fooFoo: 'foo-prop-value', - barBar: 'bar-prop-value', - }); - expect(element.fooFoo).toBe('foo-prop-value'); - expect(element.barBar).toBe('bar-prop-value'); - - // Upgrade the element to a Custom Element (without inserting it into the DOM) and set an - // attribute. - customElements.define(selector, ElementCtor); - customElements.upgrade(element); - element.setAttribute('barbar', 'bar-attr-value'); - expect(element.fooFoo).toBe('foo-prop-value'); - expect(element.barBar).toBe('bar-attr-value'); - - // Insert the element into the DOM. - testContainer.appendChild(element); - expect(element.fooFoo).toBe('foo-prop-value'); - expect(element.barBar).toBe('bar-attr-value'); - - expect(strategy.inputs.get('fooFoo')).toBe('foo-prop-value'); - expect(strategy.inputs.get('barBar')).toBe('bar-attr-value'); - }); - // Helpers - function createTestCustomElement() { - return { - selector: `test-element-${++selectorUid}`, - ElementCtor: createCustomElement(TestComponent, {injector, strategyFactory}), - }; - } - @Component({ selector: 'test-component', template: 'TestComponent|foo({{ fooFoo }})|bar({{ barBar }})',