Skip to content

Add custom element with method #2466

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

Open
wants to merge 18 commits into
base: main
Choose a base branch
from
30 changes: 30 additions & 0 deletions libraries/__shared__/webcomponents/src/ce-with-methods.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
/**
* @license
* Copyright 2017 Google Inc. All rights reserved.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

class CEWithMethods extends HTMLElement {

test() {
this.innerText = 'Success';
}

connectedCallback() {
this.test();
}

}

customElements.define('ce-with-methods', CEWithMethods);
13 changes: 11 additions & 2 deletions libraries/angular/src/basic-tests.js
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,8 @@ import {
ComponentWithProperties,
ComponentWithUnregistered,
ComponentWithImperativeEvent,
ComponentWithDeclarativeEvent
ComponentWithDeclarativeEvent,
ComponentWithMethods
} from "./components";

beforeEach(function() {
Expand All @@ -40,7 +41,8 @@ beforeEach(function() {
ComponentWithProperties,
ComponentWithUnregistered,
ComponentWithImperativeEvent,
ComponentWithDeclarativeEvent
ComponentWithDeclarativeEvent,
ComponentWithMethods
],
schemas: [CUSTOM_ELEMENTS_SCHEMA]
});
Expand Down Expand Up @@ -146,6 +148,13 @@ describe("basic support", function() {
let data = wc.str || wc.getAttribute("str");
expect(data).to.eql("Angular");
});

it('will not overwrite methods', function () {
let fixture = TestBed.createComponent(ComponentWithMethods);
fixture.detectChanges();
let root = fixture.debugElement.nativeElement;
expect(root.innerText).to.eql('Success')
});
});

describe("events", function() {
Expand Down
10 changes: 10 additions & 0 deletions libraries/angular/src/components.ts
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ import 'ce-without-children';
import 'ce-with-children';
import 'ce-with-properties';
import 'ce-with-event';
import 'ce-with-methods';

@Component({
template: `
Expand Down Expand Up @@ -105,6 +106,15 @@ export class ComponentWithProperties {
}
}

@Component({
template: `
<div>
<ce-with-methods [test]="true"></ce-with-methods>
</div>
`
})
export class ComponentWithMethods {}

@Component({
template: `
<div>
Expand Down
14 changes: 14 additions & 0 deletions libraries/react/src/basic-tests.js
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ import {
ComponentWithUnregistered,
ComponentWithImperativeEvent,
ComponentWithDeclarativeEvent,
ComponentWithMethods,
} from "./components";

// Setup the test harness. This will get cleaned out with every test.
Expand Down Expand Up @@ -198,6 +199,19 @@ describe("basic support", function () {
expect(data).to.eql("React");
});

it('will not overwrite methods', function () {
let root;
render(
<ComponentWithMethods
ref={(current) => {
root = current;
}}
/>
)
let wc = root.wc;
expect(wc.innerText).to.eql('Success');
})

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should probably mirror this to Preact too, but Preact will fail for a similar reason:

https://github.com/preactjs/preact/blob/5c9625a56b8d99e905b3beba9c09b5062fe41287/src/diff/props.js#L121-L124

This is more of a limitation in the standard JSX syntax than anything else, framework has to make a guess when it comes to attrs vs props.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be great to see this across all the libraries and frameworks so they can make an active decision towards better support or not on their own terms.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm happy to extend it to as many of them as I can manage; I've manged to get tests mostly running with git bash on windows but it's still not fully happy.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I might also reframe the test to catch other potential gotchas - readonly fields, etc.

Copy link
Contributor

@trusktr trusktr Dec 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a great start, thanks @mrginglymus!

@Westbrook Indeed, it would be great to get this implemented for all the frameworks.

Can we make a new branch for this, and then make this PR merge to that branch instead of main, that way we can get individual PRs merged piece by piece into the branch until all the tests for each framework, and then finally we can make a final PR to merge the branch into main once it is ready? Who has control of the repo to make that branch?

This is more of a limitation in the standard JSX syntax than anything else, framework has to make a guess when it comes to attrs vs props.

@rschristian Solid.js and Pota both solve this within the limitation of JSX syntax, by looking for attr:foo=, prop:foo=, bool:foo=, and on:foo=. React and Preact could do something like that too, without having to update the JSX spec, as foo:bar= is already valid JSX.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh! Here's an idea.

Perhaps, we don't have to get all tests for all frameworks implemented. We could consider any framework that's missing the test as not passing. Then this would encourage framework authors to chip in (or whoever to contribute it when they can).

Copy link

@rschristian rschristian Dec 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

React and Preact could do something like that too, without having to update the JSX spec, as foo:bar= is already valid JSX.

Barely.

foo: is a namespace, not supported by default in Babel (or any other tooling as far as I am aware) and not supported at all in many others. I wrote a PR to support it in Preact months ago but support is pretty limited for utilizing that.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For what it's worth, tsc and vite seem to pass through namespaced attrs just fine. However, I don't think we need to solve React's problem here, just ensure that it's been recorded.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh neat, good to know! I believe esbuild didn't support that in the past, unless I'm misremembering.

// TODO: Is it the framework's responsibility to check if the underlying
// property is defined? Or should it just always assume it is and do its
// usual default behavior? Preact will actually check if it's defined and
Expand Down
9 changes: 9 additions & 0 deletions libraries/react/src/components.js
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import 'ce-without-children';
import 'ce-with-children';
import 'ce-with-properties';
import 'ce-with-event';
import 'ce-with-methods';

export class ComponentWithoutChildren extends Component {
render() {
Expand Down Expand Up @@ -110,6 +111,14 @@ export class ComponentWithProperties extends Component {
}
}

export class ComponentWithMethods extends Component {
render() {
return <div>
<ce-with-methods test ref={(el) => this.wc = el}></ce-with-methods>
</div>
}
}

export class ComponentWithUnregistered extends Component {
render () {
const data = {
Expand Down
7 changes: 7 additions & 0 deletions libraries/svelte/src/basic-tests.js
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import {
ComponentWithProperties,
ComponentWithUnregistered,
ComponentWithImperativeEvent,
ComponentWithMethods,
} from "./components";
import { tick } from "svelte";

Expand Down Expand Up @@ -125,6 +126,12 @@ describe("basic support", function() {
expect(data).to.eql("svelte");
});

it('will not overwrite methods', function () {
new ComponentWithMethods({ target: scratch });
const wc = scratch.querySelector('#wc');
expect(wc.innerText).to.eql('Success');
})

// it('will set boolean attributes on a Custom Element that has not already been defined and upgraded', function() {
// new ComponentWithUnregistered({ target: scratch });
// let wc = scratch.querySelector("#wc");
Expand Down
2 changes: 2 additions & 0 deletions libraries/svelte/src/components.js
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import 'ce-without-children';
import 'ce-with-children';
import 'ce-with-properties';
import 'ce-with-event';
import 'ce-with-methods';

export { default as ComponentWithoutChildren } from './components/ComponentWithoutChildren.svelte';
export { default as ComponentWithChildren } from './components/ComponentWithChildren.svelte';
Expand All @@ -28,3 +29,4 @@ export { default as ComponentWithProperties } from './components/ComponentWithPr
export { default as ComponentWithUnregistered } from './components/ComponentWithUnregistered.svelte';
export { default as ComponentWithImperativeEvent } from './components/ComponentWithImperativeEvent.svelte';
export { default as ComponentWithDeclarativeEvent } from './components/ComponentWithDeclarativeEvent.svelte';
export { default as ComponentWithMethods } from './components/ComponentWithMethods.svelte';
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
<ce-with-methods id="wc" true></ce-with-methods>
10 changes: 9 additions & 1 deletion libraries/vue/src/basic-tests.js
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,8 @@ import {
ComponentWithProperties,
ComponentWithUnregistered,
ComponentWithImperativeEvent,
ComponentWithDeclarativeEvent
ComponentWithDeclarativeEvent,
ComponentWithMethods
} from "./components";
import { expect } from "chai";

Expand Down Expand Up @@ -147,6 +148,13 @@ describe("basic support", function() {
expect(data).to.eql("Vue");
});

it('will not overwrite methods', function () {
const app = createApp(ComponentWithMethods);
app.mount(scratch);
const wc = scratch.querySelector('#wc');
expect(wc.innerText).to.eql('Success');
})

// it('will set boolean attributes on a Custom Element that has not already been defined and upgraded', function() {
// let root = new ComponentWithUnregistered().$mount(scratch).$el;
// let wc = root.querySelector('#wc');
Expand Down
11 changes: 11 additions & 0 deletions libraries/vue/src/components.js
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import 'ce-without-children';
import 'ce-with-children';
import 'ce-with-properties';
import 'ce-with-event';
import 'ce-with-methods';

export const ComponentWithoutChildren = defineComponent({
template: `
Expand Down Expand Up @@ -100,6 +101,16 @@ export const ComponentWithProperties = defineComponent({
}
});

export const ComponentWithMethods = defineComponent({
template: `
<div>
<ce-with-methods id="wc"
:test.attr="true"
></ce-with-methods>
</div>
`
})

export const ComponentWithUnregistered = defineComponent({
template: `
<div>
Expand Down