made contexts default to new Set()

[michaelairola]

This fixes a bug I was having. "contexts" was undefined upon initialization, and while setting certain properties of the hybrid model, the cache function was trying to delete a value out of the contexts value (which was undefined). very small code change.

[codesandbox-ci]

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.

[coveralls]

Coverage remained the same at 100.0% when pulling c49d2b0 on michaelairola:undefined-contexts-bug into d6eecaa on hybridsjs:master.

[smalluban]

Thanks for the PR. With your help, we might find a use case where it needs a change. However, the real problem is with

hybrids/src/cache.js

Line 157 in d6eecaa

depEntry.contexts.delete(entry);

where I recently removed condition for checking if contexts is defined in deps.

Please look at my explanation in #94 why I thought that this condition is not required.

It would be great if you could create a test case where the library throws an error without your change. I couldn't find a situation where it can be a problem.

[michaelairola]

Thanks for the quick response, I can see where you are on this with a little more clarity. I'm not sure if this is something that should be encouraged, but I may have found a use case:

I'm playing around with a hybrid component currently, and I wanted to have a property that was connected directly to the styling of a particular node in the template. Ergo, I created a "styleProperty" factory that upon updating, will immediately change the styling in the template.
check it out here:
https://github.com/michaelairola/HighlightHelper/blob/4e7a03f30e726b5acc84582c1fba36c8293a6eba/src/utils.js#L38

The only problem is if I update one of these properties and I also update a regular property, contexts is undefined and the error throws.

As a side note, is there any benefit to initializing contexts as undefined instead of just an empty set? Initializing as an empty set gets rid of needing a guard anywhere, and to my knowledge it doesn't look like the code checks for contexts unless we are looping through it (which, if the set is empty, does nothing).

Let me know what you think, I really like this project and would love to help or understand it more :)

[smalluban]

As a side note, is there any benefit to initializing contexts as undefined instead of just an empty set?

The main reason is memory consumption. The cache mechanism is attached to every property of every instance of web components created with the library. Although the entry.contexts is only required for properties, which are dependencies of other ones, which are observed (like render() and others with observer() method defined). Because of that, it is overkill to create empty set for each cache entry, as it is used rarely.

Thanks for the code example. I will try to run it and debug the place where the problem occurs.

The only problem is if I update one of these properties and I also update a regular property, contexts is undefined and the error throws.

I am not sure if I understand what you mean with "regular property". Can you provide some basic step-by-step reproduction with your project? I can be like open the app, click here and there, boom ;)

I've found https://github.com/michaelairola/HighlightHelper/blob/4e7a03f30e726b5acc84582c1fba36c8293a6eba/src/helper.js#L55 :)

[smalluban]

Thanks to you I have found a problematic use case. The cache throws when property, which is going to be observed is called before cache.observe() setup is done. Then in the sequential invocation, its dependencies don't have contexts property set. Tada 🎉

In your case, you are calling host.render() inside of the init property, because you need template elements refs. The init property connect() method is called before the internal cache setup of the render property.

Sorry for not letting you be a contributor, but this problem also required changes in define.js file, adding some tests, so I pushed commit with fix and released v4.1.4 version. The commit has a link to this PR for a notice.

[michaelairola]

No problem, I'm glad the use case is now functional! Thanks for plugging the fix :)

[smalluban]

When I debug the problem I looked at your code, and I think some of the patterns might be done easier.

The most complex structure is for applying styles. The template engine supports passing styles to the elements within the template. You can make those values as properties, so they can be manipulated from outside. The rest "static" styles should be set inside of the <style></style> element. It is the most performant way for styling. I know that you try to set styles for the host element, but you have already div#HelperBox inside, which can take those values instead. Then your host element can be used for static styles.

For example, you can write code like this:

const HighlightHelper = {
  opacity: 0,
  top: 0,
  left: 0,
  ...,
  render: ({ opacity, top, left }) => html`
    <style>
      :host {
        position: absolute;
	boxShadow: 0 30px 90px -20px rgba(0,0,0,.3), 0 0 1px 1px rgba(0,0,0,.5);
	fontSize: 14px;
      }
      #HelperBox {
        overflow: hidden;
      }
    </style>
    <div id="HelperBox" style="${{ opacity, top, left }}">
      <div id="PageWrapper">
      ...
     </div>
   </div>
  `,
};

Your code calls host.render() multiple times, just to set styles, which can be passed directly. Using property() factory implicitly in the above example gives you one more advantage - from now, it is possible to set a default value for the element using attributes (if a user of your component would write that element in HTML). Also, styles inside of the Shadow DOM are scoped by default, so it is safe to write CSS without worry about the influence on other elements.

On another hand, you can omit passing styles at all, and use class change for "on/off" effect with smooth animation using transition or animation in CSS. It then would all you to just set bool or other simple value as property which would trigger displaying popup:

html`
  <style>
    #PageWrapper.on { ... }
  </style>
  <div id="PageWrapper" class="${{ on: !!text }}">...</div>
`

At last but not least, I would recommend creating a more declarative way of initializing the component. For example, it could be a custom element, which should wrap a subtree in DOM, where the highlight feature should work. Then put children of those elements in element inside of the element template. You can create a switch property for controlling if the popup is visible or not:

function toggleHelper(host) {
  host.text = window.getSelection().toString(); // It might be a trigger for display
}

const HighlightHelper = {
  render: ({ text }) => html`
    <style> ... </style>
    <div id="HelperBox" class="${ on: !!text }">
    ...
    </div>
    
    <slot onmouseup="${toggleHelper}"></slot>
  `,
;

Then you can use your highlight-helper everywhere you want, without the worry of unique tag name:

<body>
   <highlight-helper>
     <section>
       Some text
     </section>
   </highlight-helper>
   ...
</body>

[michaelairola]

I mean, when you put it like that... hahahaha

Yeah, I really have no defense of my code. Looking back, I guess I was trying to use class-based solutions in a framework based on objects and functions... Not my best side project implementation

Thanks for the help! I do appreciate it :)