Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

`this` reference in contructor broken #168

Closed
schrotie opened this issue Jun 16, 2019 · 14 comments

Comments

Projects
None yet
3 participants
@schrotie
Copy link

commented Jun 16, 2019

This line is the actual constructor call for a CustomElement using customElements.define:


Now anything one does to this is lost in further calls to stuff in the CE.

I write vanilla ES6, transpile with babel 7.x (current as of this writing). I can work around this issue in various places. However, I also use the polymer guys template polyfill, which does some magic to make content work, and I cannot reasonably work around that. Any suggestions?

@WebReflection

This comment has been minimized.

Copy link
Owner

commented Jun 16, 2019

Everything is in the README

@schrotie

This comment has been minimized.

Copy link
Author

commented Jun 16, 2019

I read the README several times and tried some things, but I don't see this specific case mentioned. The thing is, I develop for browsers that support CE (V1) and need to polyfill for IE. So how could I have a usable this pointer in IE?

@WebReflection

This comment has been minimized.

Copy link
Owner

commented Jun 16, 2019

AFrame and all others managed, so you just need to understand how the constructor caveat works, which is written in the README.

Don't ever assume a super returns the instance itself instead of something else, or don't use directly the constructor with custom elements.

class MyElement extends HTMLElement {
  // the self argument might be provided or not
  // in both cases, the mandatory `super()` call
  // will return the right context/instance to use
  // and eventually return
  constructor(...args) {
    const self = super(...args);
    self.addEventListener('click', console.log);
    // important in case you create instances procedurally:
    // var me = new MyElement();
    return self;
  }
}
@schrotie

This comment has been minimized.

Copy link
Author

commented Jun 16, 2019

Yes, I know this. I tried that, but it actually does nothing because the current Babel version does that automatically. I think the bug comes down to me extending the template element. Quite possibly nobody tried that with the polyfill.

I also know how I could work around that - I simply could not extend template, end of story. Doable, but I'd really, really like to be able to do that, because it enables some very nice patterns. I'm developing a "framework" for a customer based on native tech and trying hard to "keep it simple" as you keep advocating. BTW, thanks a lot for all your thoughts and code! You make a lot of sense on several topics and I'm trying to follow some of that.

So if you tell me to forget it and go on without smart templates, I'll do that. So, no way?

@schrotie

This comment has been minimized.

Copy link
Author

commented Jun 16, 2019

Oh, by the way, I believe this problem is somewhat deeper and may be connected to CE built in extend in general. None of the examples do something like this.foo = "bar". All just work with the DOM. And the successful projects your quote use the V0 API, right? So they did something else entirely. You mention in your Safari polyfill, that the constructor isn't usable. Does the same apply to this polyfill?

@WebReflection

This comment has been minimized.

Copy link
Owner

commented Jun 16, 2019

look, I've zero code example of what you are trying to do, so it's super hard to answer anything in here.

AFrame switched to V1 without issues just recently, this.foo = 'bar' is covered if you use the pattern described in the README.

If you don't have a minimal example of what is failing for you, I have no idea how to help.

@WebReflection

This comment has been minimized.

Copy link
Owner

commented Jun 16, 2019

P.S. I use V1 in heresy and extend every builtin without issues ... so maybe let's start from where your code fails your expectations, so I can try to see if there's anything to fix, or any workaround.

@MikeVaz

This comment has been minimized.

Copy link

commented Jun 16, 2019

Hey @schrotie we extend built-ins in our components and ship it to production. But we don't use templates.
I might be able to help you since we faced similar issues and found workarounds. Andrea has developed a dedicated polyfill for that which works well in our case. https://github.com/ungap/custom-elements-builtin. You need to use it besides Custom Elements polyfill.

This would help you to polyfill Safari/IE/Edge. And builtin polyfill have a nasty caveat arround using constructor mentioned above. So you need to use a workarround.

If you extend Template element this would bring another layer of complexity in IE. Because it is not natively supported in IE and will be polyfilled too.

I would suggest the following steps:
1.) Try extending simple element like HTMLButtonElement
2.) Make it work in Safari first avoiding Babel.
3.) Set up Babel properly to work in this case. Transpilled code should work in Chrome and Safari. first.
4.) Try it in IE11.
5.) Try extending Template element and make it work in Safari following the same steps as for HTMLButtonElement
6.) Try it in IE11 with template polyfill.

Share your code so we could give you more tips.

@schrotie

This comment has been minimized.

Copy link
Author

commented Jun 16, 2019

Thanks a lot Guys!

@MikeVaz Yeah, I'm aware of the Safari issue, but kept pushing it to the back of my head. I was quietly hoping that I could just dump the documents-register-element polyfill and force it in Safari. But Safari was my second concern and I was working on IE first, since Safari is not currently required (sure, it will be, I know).

@WebReflection Good call! So I assembled a minimal test case of the problem and that got me one step further. You are right, everything works perfectly on your side. This may actually be an integration problem between babel and this polyfill. I attached my test case. To reproduce unzip polyfillTest.zip and then:

cd polyfillTest
npm install
npm build
npm start

and point browser to localhost:5000

Chrome will load native, IE transpiled (yes, I'll make the script loading more clever eventually). The test contains four examples. The first three are buttons, the last a template. All work in Chrome, the first two in IE.

I believe the crucial failure is already captured in the third test: It is a simple button that does almost the exact same thing as the first two (which work). However, the button extends an intermediate class that extends button. And between these two the this pointer breaks.

Maybe the polyfill does something weird with inheritance chains. However, on my debugging spree I saw that babel may not be fully transparent with regards to this. In cases where I expected it to access a property from my base class it only considered my immediate prototype and not the whole chain. That might also be related. I could be totally off, though, it just an impression I had, not something I would bet any significant amount of money on.

@WebReflection

This comment has been minimized.

Copy link
Owner

commented Jun 16, 2019

@schrotie this works for me:

if (!('content' in HTMLTemplateElement.prototype)) {
	var content = new WeakMap;
	var setContent = function (self) {
		var fragment = self.ownerDocument.createDocumentFragment();
		while (self.hasChildNodes())
			fragment.appendChild(self.firstChild);
		content.set(self, fragment);
		return fragment;
	};
	Object.defineProperty(
		HTMLTemplateElement.prototype,
		'content',
		{
			configurable: true,
			get: function () {
				return content.get(this) || setContent(this);
			}
		}
	);
}

customElements.define('ie-this', class extends HTMLButtonElement {
	constructor(...args) {
		super(...args);
		this.foo = 'Hello this-button!';
	}
	connectedCallback() {this.innerHTML = this.foo;}
}, {extends: 'button'});


// Not currently necessary, babel 7.4 does this anyway!
customElements.define('ie-self', class extends HTMLButtonElement {
	constructor(...args) {
		const self = super(...args);
		self.foo = 'Hello self-button!';
		return self;
	}
	connectedCallback() {this.innerHTML = this.foo;}
}, {extends: 'button'});

class FirstLevelButton extends HTMLButtonElement {
	constructor(...args) {
		const self = super(...args);
		self.foo = 'Hello first level';
		return self;
	}
}

// Not currently necessary, babel 7.4 does this anyway!
customElements.define('ie-second-level', class extends FirstLevelButton {
	connectedCallback() {this.innerHTML = this.foo;}
}, {extends: 'button'});

customElements.define('ie-template', class extends HTMLTemplateElement {
	constructor(...args) {
		return super(...args);
	}
	connectedCallback() {
		this.parentElement.appendChild(this.content.cloneNode(true));
	}
}, {extends: 'template'});

You might have a mixture of issues there:

  • for some reason I don't understand, the HTMLTemplateElement polyfill you use is not providing content at the prototype level ... kinda a non-sense to me
  • Babel might do something funny when classes are not statically native ... never trust Babel fixing the constructor dance for non native classes (there is already a bug for that)
@WebReflection

This comment has been minimized.

Copy link
Owner

commented Jun 16, 2019

P.S. when you do nothing in a constructor, you don't need to provide one or do anything at all

@MikeVaz

This comment has been minimized.

Copy link

commented Jun 16, 2019

My point of testing in Safari first is to exclude Babel issues alltogether from the picture since Safari supports ES2015 classes natively and you don't need to transpile your code. This way you will be just focusing on the polyfill problems excluding other factors.

@schrotie

This comment has been minimized.

Copy link
Author

commented Jun 16, 2019

@WebReflection
Hey Andrea, thanks a lot, this is really awesome! Please let me know, if I can do something in return.

@WebReflection

This comment has been minimized.

Copy link
Owner

commented Jun 17, 2019

@schrotie no problem, watch out I've changed the code a bit, so you can access content N times instead of only once

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.