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

<svelte:element> proposal implementation #3928

Closed
wants to merge 2 commits into from
Closed

Conversation

erzr
Copy link

@erzr erzr commented Nov 15, 2019

An implementation of the proposal described in #2324.

Problem:
There's currently no way to dynamically specify a tag for an element. This is problematic if you want the ability to add a tag prop to a component and have some or all of that component be wrapped in the tag specified. Workarounds for this issue result in large, incomplete wrapper components such as https://github.com/erzr/jss-svelte/blob/master/packages/jss-svelte/src/components/TagWrapper.svelte that just cover the most commonly used tags.

Solution:
A new element named svelte:element has been implemented that accepts a single prop tag, which could be a string or variable containing the tag name.

Usage:
<svelte:element tag="h1">This is an h1</svelte:element>

@antony
Copy link
Member

antony commented Nov 25, 2019

@erzr I can't comment on the proposal itself as I've never had need for this, but this PR could definitely use some tests.

Edit: I'm beginning to understand what this is about, and if it is accepted, I think we could re-use the existing

<svelte:component tag="blah" this={YourComponent} />

rather than creating a slightly confusing new <svelte:element> tag.

@erzr
Copy link
Author

erzr commented Nov 25, 2019

@antony Thanks for the feedback. Definitely do not disagree with the tests feedback. I didn't spend any time writing them because I was unsure if those proposal was something that Svelte was interested in as a whole.

I did consider re-purposing <svelte:component/> and I also did consider <svelte:options/> but ultimately settled on creating a new tag because I felt it aligned more closely with the Element internally:

https://github.com/sveltejs/svelte/blob/ea66d52f2b1718a77d0457a4ba77438291cb9f00/src/compiler/compile/nodes/Element.ts

vs

https://github.com/sveltejs/svelte/blob/ea66d52f2b1718a77d0457a4ba77438291cb9f00/src/compiler/compile/nodes/InlineComponent.ts

Considering <svelte:element/> internally as an Element would allow all of the existing logic that is applied to any other regular element to also be applied to this new tag. For example, any tag specific warnings:

if (!a11y_required_content.has(this.name)) return;
if (this.children.length === 0) {
this.component.warn(this, {
code: `a11y-missing-content`,
message: `A11y: <${this.name}> element should have child content`
});
}

Open to suggestions here. If this gets positive feedback and has a chance of getting merged in, I'm happy to spend the time writing tests to cover the new functionality.

@pngwn
Copy link
Member

pngwn commented Nov 26, 2019

I haven't looked at the code yet but I agree that this should be its own special tag, I don't think it makes sense to use svelte:component here, they are quite different in terms of implementation and the difference in usage is distinct enough to warrant a new API in my opinion.

@matthewrobb
Copy link

I am curious if this is being looked at seriously or not. This is currently an issue I keep running into when trying to build highly general purpose UI Library style component sets. Even the simple case of wanting to create a component that could be any of a number of tags (a, button, input[type=button|submit]) requires quite a bit of boilerplate where this proposal would make it mostly very trivial.

If the concern is baking in special case code for things like setting attributes then possibly requiring a staticly declared list of possible tags's would suffice?

<!-- is="" being a compile time only prop supporting a static set of valid tag names -->
<svelte:element tag={'a'} is="a, button, input" />

@arggh
Copy link
Contributor

arggh commented Feb 20, 2020

I keep needing this feature three times a week.

@adamduncan
Copy link

adamduncan commented Mar 20, 2020

I can see a few scenarios where this would be beneficial. Having the ability to augment some component's tagName based on its use can be very helpful in maintaining proper semantics. (E.g. styled-components and other React libraries do this with their as prop)

<Box as="article">

<Heading as="h2">

<List as="ol">
   ...

Here's a super naive implementation of a generic Element component. It just monkey-patches props and clobbers the DOM (doesn't take into account any events, directives, etc.), but might help to illustrate the goal here:
https://svelte.dev/repl/77040fcad0ad4a9c9d19ca74aee04c2d?version=3.20.1

Could svelte:component or a new element perhaps support this use case? (Or are conditional approaches enough for now?)

@pngwn
Copy link
Member

pngwn commented Mar 22, 2020

The feature is highly likely to be implemented, the API and implementation are the only real topics of discussion right now.

@pd4d10
Copy link

pd4d10 commented Apr 4, 2020

Need this feature +1. Is there anything I can help with?

@jhoffmcd
Copy link

I definitely would reach for this, especially in the creation of component libraries for use in a design system. What is holding this PR up from approval, just the merge conflict? What is outstanding? I would love to help out as well.

@jonatansberg
Copy link

I think extending the svelte:component syntax in a way so that it accepts either a component or a tag name would be interesting venue to explore. That way it would be possible to swap out a plain html element for a full component that would render a different (or enhanced) version of that plain element.

It would also make it easier to deal with different forms of structured JSON content (from CMSs like Contentful, Primic, etc) where you can embed rich elements inside the text block/wysiwyg field types.

https://www.contentful.com/developers/docs/concepts/rich-text/

@benmccann
Copy link
Member

It looks like there's another implementation of this here: #5481

@erzr
Copy link
Author

erzr commented Dec 21, 2020

I'm going to close this because #5481 is a more complete implementation and has been reviewed more at this point. Hopefully it'll be adopted soon.

@erzr erzr closed this Dec 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet