Add custom element with method #2466
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
libraries/react/src/basic-tests.js
Outdated
| it('will not overwrite methods', function () { | ||
| let root; | ||
| render( | ||
| <ComponentWithMethods | ||
| ref={(current) => { | ||
| root = current; | ||
| }} | ||
| /> | ||
| ) | ||
| let wc = root.wc; | ||
| expect(wc.innerText).to.eql('Success'); | ||
| }) | ||
|
|
There was a problem hiding this comment.
Should probably mirror this to Preact too, but Preact will fail for a similar reason:
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I might also reframe the test to catch other potential gotchas - readonly fields, etc.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Oh neat, good to know! I believe esbuild didn't support that in the past, unless I'm misremembering.
mrginglymus
force-pushed
the
ce-with-methods
branch
from
dbfc970 to
40e87e6
Compare
…s with unwriteable properties
mrginglymus
force-pushed
the
ce-with-methods
branch
from
40e87e6 to
e359f1c
Compare
|
I'm a little confused to as to how I'm supposed to add a successfully failing test... |
mrginglymus
force-pushed
the
ce-with-methods
branch
from
820f51b to
7e7d1e7
Compare
|
This PR now includes tests for all currently supported frameworks. There are many that have now dropped from 100%, mostly I think those using JSX-like syntax. I've tried to check each one's documentation for hints as to how to set attrs rather than props. |
React 19 has a very naïve heuristic for setting props vs attributes; if it detects a property on the custom element with the same name as the attribute, it sets it as a property. This doesn't work if you have a method on the custom element with the same name as an attribute you are trying to set.
I've attempted to provide a reproduction here; unfortunately this repository does not behave well on Windows or WSL, which are currently my only two options. I will investigate adding support for Windows devs in another PR.