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

using Skate without Shadow DOM/Shadow Roots? #802

Closed
priley86 opened this issue Sep 16, 2016 · 19 comments

Comments

Projects
None yet
6 participants
@priley86
Copy link

commented Sep 16, 2016

@treshugart thanks for contributing this library, it's a wonderful implementation.

My only question is are there any plans to optionally render custom elements without shadow DOM/shadow Roots? This would open up this library to potentially many more audiences and use cases. To add some context to this, many teams who have previously developed pattern libraries in things like jquery/plain old css and have strict browser requirements will be slowly adopting new custom element strategies. Rob Dodson's post details this migration strategy well:

jquery/css => CEs => CEs + Shadow DOM (when browser support improves)

What would be nice is to be able to use this library and take advantage of Incremental DOM now (while not adopting Shadow DOM until later).

Would you potentially explore this? Or maybe a fork of this could?

@treshugart

This comment has been minimized.

Copy link
Member

commented Sep 18, 2016

We need to unhide this in the docs:

Skate will work without Shadow DOM support, but you won't be able to compose components together due to the lack of DOM encapsulation that Shadow DOM gives you.

So you can, you just can't add content to your components as light DOM. See http://www.webpackbin.com/NJKsxfdnW. The only issue comes when you want to add light DOM to a component; Incremental DOM will overwrite any content added to your element that isn't in render() because there's no shadow root to encapsulate your component.

@priley86

This comment has been minimized.

Copy link
Author

commented Sep 19, 2016

@treshugart thanks for this response, this is helpful to know that Skate would remove any light DOM. After looking at your webpackbin, I suppose I should follow up with another question.

Is there anyway to configure Skate to not create shadowRoots for the custom element's nested markup? While it may be feasible to style the host element, having the option to still style the nested markup/rendered vdom with external/global styles would still be a nice add (for some teams). I know the better approach is to use Shadow DOM and encapsulate CSS (and I know you've proposed using CSS modules), but for teams which have strict browser requirements and are exploring CEs for JS/HTML encapsulation only, having the option to remove shadowRoots would make Skate applicable (this way external css can still be used).

@treshugart

This comment has been minimized.

Copy link
Member

commented Sep 21, 2016

I'm not sure if I'd want to add a configuration option to do it as it's a fairly fundamental benefit of using web components. That said, a way you could do this now would be to override attachShadow() to not do anything and to return the element:

import { define } from 'skatejs';

define('x-test', {
  prototype: {
    // This should be fine for most browsers now
    attachShadow() {
      return this;
    },

    // However, in old Chrome / Opera or old polyfills
    // you might need to do this.
    createShadowRoot() {
      return this;
    }
  }
});

If you find yourself having to do this a lot, you can create a base class that extends skate.Component, or use a composable function:

function noShadowDOM(opts = {}) {
  return Object.assign(opts.prototype || {}, {
    attachShadow() {
      return this;
    }
  });
}

define('x-test', noShadowRoot({
  render() {}
});
@priley86

This comment has been minimized.

Copy link
Author

commented Sep 23, 2016

Thanks so much for this @treshugart, this is exactly what we needed to know!

@priley86 priley86 closed this Sep 23, 2016

@bedeoverend

This comment has been minimized.

Copy link
Contributor

commented Nov 18, 2016

@treshugart in overriding attachShadow - can you see any dangers in doing that? e.g. a library talking to elements in general and expecting shadow roots?

@bedeoverend

This comment has been minimized.

Copy link
Contributor

commented Nov 18, 2016

also would skate ever consider the option of having two render pipelines? Not sure how you'd implement that, but in the case of an element that wants to have perhaps complete control over it's light DOM based on its props, but also needs Shadow DOM?

@treshugart

This comment has been minimized.

Copy link
Member

commented Nov 18, 2016

@bedeoverend

in overriding attachShadow - can you see any dangers in doing that? e.g. a library talking to elements in general and expecting shadow roots?

Possibly, but the library that's talking to those elements should check for a shadowRoot property. However, since it wouldn't exist if { mode: 'closed' }, then I'm not sure there'd be a reliable way for an external library to do this unless they overrode attachShadow, too. That could be troublesome, for sure.

also would skate ever consider the option of having two render pipelines? Not sure how you'd implement that, but in the case of an element that wants to have perhaps complete control over it's light DOM based on its props, but also needs Shadow DOM?

I'm probably projecting here, but could you be talking about custom form elements :p? I've definitely seen a need for that. Maybe this is a good path to that? What is your use case here? cc @bengummer

@bedeoverend

This comment has been minimized.

Copy link
Contributor

commented Nov 18, 2016

@treshugart it's certainly an edge case, but yeah it feels a little odd overriding the attachShadow - not sure if it's in Skate's scope, but perhaps a toggle for where to render to? Or overriding the render function on Skate's base component class even? Interestingly - @matthewp just floated this as an idea on his Bram library.

Re: rendering to both - haha, well actually I was thinking of our Simpla elements - the elements themselves are designed to have complete control over their children based on their props / internal state, I really like Skate's render pipeline but would obviously want to render into light dom. There's also however the benefit of hiding the element's controls away inside its Shadow DOM, so being able to render into both based on prop changes would be 👌

@bengummer

This comment has been minimized.

Copy link
Contributor

commented Nov 18, 2016

@treshugart yeah having separate render() functions for light + shadow DOM would allow the element to expose form elements in light DOM (so things like native form behaviour work as expected).

Once there is official support for custom elements exposing their own form values (spec discussion) this dual-render functionality wouldn't be needed for forms anymore.

If native form behaviour is the only use case here then it may be better to just document the attachShadow workaround (i.e. inserting form elements into light DOM so the <form> can see them). If there are other uses cases then maybe dual-render would be worth it.

@treshugart

This comment has been minimized.

Copy link
Member

commented Nov 18, 2016

Once #872 is merged, we'll officially document the rendererCallback() which can be used to implement a custom renderer. It'd be up to the renderer to call renderCallback(), so it could have an alternate render, or do whatever it wants at that point. This is still undocumented, though, as I still have questions around how to make the Incremental DOM renderer optional. This will probably happen through mixins, where you'd have a RenderMixin() that an IncrementalDOM mixin extends. The base RenderMixin() could just enforce that you provide it a renderer.

@matthewp

This comment has been minimized.

Copy link
Contributor

commented Nov 18, 2016

Biggest reason I see to render to the light DOM is for styling (as ironic as that is). Until @apply is a real thing theming in shadow DOM is going to be hard, and for small projects without a full time designer it might be more trouble than it's worth. For components that don't need distribution, I don't see a disadvantage to rendering to light DOM.

@treshugart

This comment has been minimized.

Copy link
Member

commented Nov 18, 2016

@matthewp the only issue there is hiding the shadow root template from any parent virtual DOMs that may have rendered the component with light DOM that would normally be slotted. In most implementations if you render() to the component as light DOM, it will be removed by the diffing algorithm. In order to do this, you'd need to fake childNodes and anything else the algorithm is interested in. I can see this as being implemented as a mixin or something.

@alexlafroscia

This comment has been minimized.

Copy link
Contributor

commented Feb 7, 2017

is there a way to do this with the current (at time of writing, 4.6.6) version of Skate? I tried overriding attachShadow as mentioned above but it throws a JS error:

Uncaught TypeError: Cannot read property 'ownerDocument' of null
    at e (index-with-deps.min.js:1030)
    at Function.value (index-with-deps.min.js:1)
    at HTMLElement.value (index-with-deps.min.js:1)
    at HTMLElement.value (index-with-deps.min.js:1)
    at MutationObserver.<anonymous> (index-with-deps.min.js:1030)
@treshugart

This comment has been minimized.

Copy link
Member

commented Feb 8, 2017

@alexlafroscia yeah, my code above wasn't exact. I've created a working example for you that uses it like a mixin so it's easy for you to share. The code above should have taken into account both ways to access the shadow root (property and function return value).

@alexlafroscia

This comment has been minimized.

Copy link
Contributor

commented Feb 8, 2017

Ahh, very interesting! Thanks. I've been trying to get a server-side-rendering story put together and disabling the Shadow DOM is a req for that.

@priley86

This comment has been minimized.

Copy link
Author

commented Aug 20, 2017

@treshugart we may be considering Skate.js again for some of our new components after reviewing the Angular integration example here:
https://github.com/Hotell/angular-skate-talk-demo

This is quite a compelling demo. The only issue I have there is still the required use of slot/shadow roots for light DOM scenarios (transcluded content like the <img>). It works great in new browers, but we still have the need to support CEs in cases w/o Shadow DOM (the original intent of this issue).

It looks like the working example/jsbin link above is no longer working. Can you provide this sample once more?

I've noticed when implementing things like conditional rendering/event handlers inside the light dom with JSX gets really hairy with Mutation Observers (i.e. you have to move elements with appendChild if actually transcluding them to maintain event listeners). It would be great to solve this problem with Skate JS and write component APIs that are very flexible to whatever the user wants in the light DOM...

ex w/ Mutation Observer (remove Secondary button fails b/c element has been moved and JSX can no longer find it):
https://stackblitz.com/edit/using-pf-tabs-in-reactjs?file=index.js

the pf-tabs CEs are implemented here:
https://github.com/priley86/patternfly-webcomponents/tree/pf-tabs-updates/src/pf-tabs

@priley86

This comment has been minimized.

Copy link
Author

commented Aug 21, 2017

It looks like real-dom implemented here may help in conjunction w/ using a custom renderer in the render callback?
https://github.com/skatejs/val#real-dom

essentially, what's desired is a way to maintain event bindings and conditional rendering / etc within the light dom (i.e. JSX rendered within the Web Component by the consumer). React makes this quite easy as children can still be flexibly modified/bound to.

@treshugart

This comment has been minimized.

Copy link
Member

commented Aug 21, 2017

@priley86 you can definitely use real-dom from val, but you just won't get the DOM diff algo from Preact. If you're rendering to light DOM, it's likely you can't do this anyways because of it becoming out of sync with it's view of the world.

@treshugart

This comment has been minimized.

Copy link
Member

commented Aug 21, 2017

JSBin is down for me, so I can't check that example, unfortunately.

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.