Use dom-context for event contract

[loganvolkers]

There are a number of libraries that use a similar dom context approach as proposed by Justin Fagnani in 2016, see the readme here: https://github.com/saasquatch/dom-context/tree/v1

Looking to centralize all the libraries on a standard event contract.

Opening this pull request to see if you're open to the collaboration.

The goal is to keep your external API intact so it doesn't create any breaking changes for your consumers.

[mihar-22]

Ooo love it ... super interested, it's definitely something that's needed in the custom-elements space. I skimmed through the source code in dom-context and your PR, looks really good to me so far. I'll have a second look later just incase, if you need anything from me ask away. When you're good on your end I can review/test on my end and merge.

[mihar-22]

I also would be open to deprecating this library and we just add a stencil package to your repo. @dom-context/wc and @dom-context/stencil maybe? Up to you.

[loganvolkers]

Good questions to think about. I'll be taking another crack at this tomorrow, focusing on getting some tests in place. I like the idea of a stencil wrapper, too. For stencil the key thing is it needs to get wired into state or props. I was planning to use state, wormhole uses props, both have some pros and cons.

…

On Tue, Sep 8, 2020, 7:51 PM Rahim Alwer ***@***.***> wrote: I also would be open to deprecating this library and we just add a stencil package to your repo. @dom-context/wc and @dom-context/stencil maybe? Up to you. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#1 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAI2PXRJ55IJ5VDDSVAJKDTSE3UTVANCNFSM4RAZDRGA> .

[mihar-22]

I like both approaches (state is cleaner) but my only issue with passing state in directly was that it creates tight coupling and makes it hard to test. I think having both is an ideal approach to cater for different use cases.

[loganvolkers]

There is also the approach that stencil-context uses, with separate Provider and Consumer components: petermikitsh/stencil-context#5

I feel like a couple helper methods like connectToProps and connectToState could be easily created.

The thing that stencil-wormhole and stencil-context are both missing that wc-context has is the ability to have multiple context namespaces. In any wrapper we develop, that will be a design goal.

For example:

import { createContext } from "@dom-context/stencil";

export const UserContext = createContext<User>("context:user");
export const RouterContext = createContext<string>("context:currentRoute");

// Connect to props
RouterContext.connectToProps(Component, "propName1");

// Connect to state
UserContext.connectToState(Component, (comp,context) => comp.statefield = context);

I'll be working on this actively at my job for the next few weeks to connect dom-context with some Stencil 1.7 components, we'll see if there's a good model that we come up with.

[loganvolkers]

@mihar-22 I have an implementation based roughly on your wrapper implementation https://gist.github.com/loganvolkers/9453eb3b92bb69454f874f874f586303

You need access to the provider so that you can send new values down, but you don't really need direct access to the listener (only the new context values).

Thoughts?

[mihar-22]

You've done a really good job everything looks perfect to me, I have nothing to add to the implementation. I only thought twice about:

context.connectToProps(StencilConsumer, (c, next) => (c.context = next));

Only because in my own experience I was directly mapping the context to properties most the time, and I wonder if it comes up for others a lot. Maybe including a utility function like so...

const mapToProps = (props?: string[]) => 
 (component: Constructor<C>, context: T)  
   => (props ?? Object.keys(context)).forEach(prop => { component[prop] = context[prop]; });

Usage

// 1.
context.connectToProps(StencilConsumer, mapToProps());

// 2.
context.connectToProps(StencilConsumer, mapToProps(['prop1', 'prop2']));

It's mostly developer preference I guess and it's situational, I was doing it mostly to @Watch props separately.

Thanks for the awesome work!

[loganvolkers]

Not against a convenience method, but some feedback on your proposal:

a) This would only work well with object contexts, not if the context is just a number, string, or function
b) should use type OverlappingKey<T,C> = keyof T & keyof C instead of string for better key type safety
c) i prefer rest arguments (...restOfName: OverlappingKey<T,C>[]) so you get context.connectToProps(Consumer, "prop1","prop2")

I see there being a benefit here for efficient re-rendering. If you're using a large context object, but only listening on a few keys, then you're not going to trigger a stencil re-render on every context change.

I don't like the idea of spreading the entire context object into all the props, though (context.connectToProps(StencilConsumer, mapToProps())). That seems like a recipe for disaster if someone inadvertently adds the wrong thing to context, then their colleagues doing the subscribing will have no idea (and no static typing) to warn them that their props will now start to be overridden by this new value.

[mihar-22]

Right... thanks for feedback! For some reason I was reflecting on my implementation when I wrote it, after reading what you said the mapToProps is a poor suggestion. I couldn't add extra type safety to stencil-wormhole because the context was created inside the provider creation method and the connectToProps was created elsewhere. In addition, my library only allowed a single object to passed down, whereas dom-context allows multiple contexts of varying types.

For the use case where someone has a large context object and wants to avoid inefficient re-renders you suggestion seems the way to go 👍