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

feat(html): scoped elements #122

Closed
wants to merge 1 commit into from
Closed

Conversation

smalluban
Copy link
Contributor

@smalluban smalluban commented Jul 21, 2020

It's rather a naive implementation of the scoped elements definition to check if it is possible to implement similar to https://open-wc.org/scoped-elements/ in hybrids.

import { html } from "hybrids";
import ScopedElement from "./ScopedElement";

const MyElement = {
  render: () => html`
    <scoped-element></scoped-element>
  `.scope({ ScopedElement }),
};

It's naive because of simple replacement of the element's names - it should be smarter, as that substring might be used in different context inside of the template string.

@codesandbox-ci
Copy link

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 3498861:

Sandbox Source
Hybrids web components playground Configuration

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 3498861 on feat-scoped-definitions into 4c24bec on master.

@riovir
Copy link

riovir commented Jul 22, 2020

I've tried the sandbox out. As long as I don't attempt to set props, it seems to work as expected.

Here is the sandbox I've tried out. When a property is set / changes it seems to try finding the element by its unscoped name to adjust it.

@smalluban
Copy link
Contributor Author

Yeah, It was a quick POC, as you can see in the code the regexp only replaces elements without attributes.

However, I have more and more concerns about putting this into the library. It would have to work on strings, which might be buggy, as parsing html is complicated thing. The second thing, it will work only on particular html template instance. For example, if your main template would have iteration with nested templates (like items.map(...)) the nested template will not have scope from the parent, it will have to be set again explicitly.

@riovir
Copy link

riovir commented Jul 24, 2020

Makes sense. I'll need some time to gather my thoughts on how a future proof API would best support the use case of scoped child components. (Assuming the direction of scoped CustomElementsRegistry progresses. There still needs to be a way in the APIs to express when and how scoping is meant to be used.)

I'll try to think up something where the existing API could be used to make something like this happen. A smaller, independent question/request:

Would that be okay to offer a function next to define that only does the definition -> class conversion? If I could take manual control over when and how customElements.define is called (if at all, ie for publishing a side-effect-free component) then I could sort out a significant chunk of the problems around sharing components.

import { toCustomElement /* or toClass */ } from 'hybrids';
import { MyElementDefinition } from '...';

export const MyElement = toCustomElement(MyElementDefinition);

(I'm a bit slow in reacting due to constantly being dragged away from the PC in these weeks, pardon about that.)

@smalluban
Copy link
Contributor Author

Would that be okay to offer a function next to define that only does the definition -> class conversion?

I think it should be rather simple. It could be even the same define() function, but taking other arguments. However, if you plan to use on those a scoped elements feature from open-wc, than it won't work, as it requires a class, which extends LitElement (it overwrites a render method using lit-html).

Do you see another use case for that feature?

@riovir
Copy link

riovir commented Jul 24, 2020

However, if you plan to use on those a scoped elements feature from open-wc, than it won't work, as it requires a class, which extends LitElement (it overwrites a render method using lit-html).

Aye, the first thing I checked was if somehow the mixin in question works with any webcomponent whatsoever. But indeed, checking the source revealed the coupling between open-wc and lit.

Do you see another use case for that feature?

I do. Just because hybrids doesn't support automagic scoping (my personal convenience), I could (actually would prefer to) still use it to compose a component pack to be consumed by other webcomponent-compatible apps.

I can take steps so that the components in my package don't refer to each other by tag. I could offer the package as a library of side-effect-free, standalone component classes in this form. The fact that I chose hybrids is an implementation detail. All my consumers see is a tree-shakeable package offering them self-contained components under any name they choose.

A bit more context on scoping. At my company, we are working towards a way to allow independent dev teams to wire in code to a single app. We chose ESM and webcomponents as the common language of integration. Reused components are distributed also as webcomponents. The tricky part is that the LitElement based outer frame (navbar, footer, slot in the middle) may use a certain version of a webcomponent. The current page content (also a webcomponent, eg. a Vue.js app wrapped with the @vue/web-component-wrapper may also use a different version of the same webcomponent). If those components would self-register, they would clash. However, if the shared components are merely classes, consumers have the tools to prevent the collision.

@smalluban
Copy link
Contributor Author

Ok, that makes sense to me, so I've created a #123, you can test it out. I have a long waiting PR (#85) which I would like to release in one batch, so this one will have to wait a few days to be released. Is it ok for you?

@riovir
Copy link

riovir commented Jul 25, 2020

Absolutely, thanks!

@riovir
Copy link

riovir commented Jul 27, 2020

Update 3: Added a simple (scopes provided upfront only) and a deluxe (supports additional scopes) implementation to address child template usage verbosity.

Update 2: Added iteration to show the clunkiness of my current API PoC. Thinking about how to eliminate it...

Update 1: Fixed a mistake breaking the page if a component re-renders.

I've played some more with scoping. This time took a swing at adding scoping without changing the API.

I've cooked up this PoC. If I'm not mistaking Hybrids doesn't allow dynamically setting the tag name. This constraint is actually a boon for this approach, expecting tag names existing in the static template fragments to be findable.

Limitations

The idea relies on rewriting the template literal fragments. Because of this, it doesn't play well with dynamically computed tag names.

Usage

import { html, define } from "hybrids";
import { FontAwesomeIcon } from "@riovir/wc-fontawesome";
import { faAnkh } from "@fortawesome/free-solid-svg-icons";
import { scopeWith } from "./scope-support";

/** Intends to be identical to html, but returns object with a scope function that needs to be called */
const scopedHtml = scopeWith({ html, define });

export default {
  render: () =>
    scopedHtml`
      Hello
      <font-awesome-icon icon=${faAnkh} size="2x"></font-awesome-icon>
    `.scope({ FontAwesomeIcon }) // Note that this comes from scope-support
};

@smalluban
Copy link
Contributor Author

smalluban commented Jul 27, 2020

I will look at your POC in my spare time.

I remembered that some time ago I've started working on app tools for hybrids, where I wanted to render dynamic elements with a list of props using a built-in HTML template engine. At first, the tagged template literals might look magical, but after all the html is nothing more than a function, which takes a list of strings and values...

Here you have the implementation (https://github.com/hybridsjs/app-tools/blob/9ae2bd15b8537bafb809178ea7c277438ff4d0cf/src/utils.js#L52):

import { html } from "hybrids";

const RENDER_PLACEHOLDER = `{{prop-${Date.now()}}}`;

export function renderElement(tagName, props) {
  const args = [];
  let template = `<${tagName}`;

  Object.keys(props).forEach(key => {
    template += ` ${key}="${RENDER_PLACEHOLDER}"`;
    args.push(props[key]);
  });
  template += `></${tagName}>`;

  return html(template.split(RENDER_PLACEHOLDER), ...args);
}

@riovir
Copy link

riovir commented Jul 27, 2020

Indeed! Corrected wording of the POC adding a limitations section.

@smalluban
Copy link
Contributor Author

I am going to close this PR. Like I said before, the current implementation is buggy, and what I can see, you have managed to make it work without changes in the library :)

Feel free to comment here or create an issue if you would have questions about the problem.

@smalluban smalluban closed this Aug 4, 2020
@smalluban smalluban deleted the feat-scoped-definitions branch August 4, 2020 07:01
@riovir
Copy link

riovir commented Aug 4, 2020

Indeed, this would mirror how LitElement + OpenWc addressed the topic. Once I have some more time, I'll look into brewing up a standalone lib to add scoping to Hybrids. Unless someone else beats me to it. :) Thanks for looking into the issue!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants