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

Attrs initialization without @attr decorator #207

Open
kristoforsalmin opened this issue Apr 29, 2022 · 4 comments
Open

Attrs initialization without @attr decorator #207

kristoforsalmin opened this issue Apr 29, 2022 · 4 comments
Labels
Milestone

Comments

@kristoforsalmin
Copy link

Hi there 😄

After upgrading from v1.1.4 to v1.3.2, I noticed that my attributes were no longer shadowed by initializeAttrs(). I started digging and stumbled upon this piece of logic introduced in #191:

if (initialized.has(instance)) return

It prevents initializeAttrs() from doing anything the second time around, thus skipping the manual initialization of attributes. Also, it seems like this change was one of the main things addressed by the above-mentioned PR.

So, what would be the correct way now to initialize the attributes without the @attr decorator?

Thanks!


Example

<script>
  class InfoMessage extends HTMLElement {
    open = true

    connectedCallback() {
      initializeAttrs(this, ['open'])
    }
  }

  controller(InfoMessage)
</script>

<info-message data-open="false"></info-message>
  1. controller(InfoMessage) wraps original connectedCallback() and calls:

initializeAttrs(instance)

  1. InfoMessage is marked as initialized (attrs.ts):

initialized.add(instance)

  1. The original connectedCallback() is executed, but initializeAttrs() would hit that early-return condition.
  2. this.open is stuck in its default state (true) 😔
@keithamus
Copy link
Member

In the guide we mention calling defineObservedAttributes. I notice your example doesn't do that. Does your example work when that is called?

@kristoforsalmin
Copy link
Author

Hey @keithamus!

Yes, you're right. I'm not calling defineObservedAttributes() as in the example. There are 2 things:

  1. initializeAttrs() used to work without calling defineObservedAttributes() in v1.1.4. My use case was that a component has some properties, but they needed no observation. It was OK to just initialize them, so that this.open returns a value form data-open, or a default value defined in the component's class itself open = true. I knew it's a risk to utilize just one part and not the other, but initializeAttrs() was doing the exact thing it promised: taking the values from the DOM and defining them on an instance (shadowing the default values along the way).
  2. Unfortunately, calling defineObservedAttributes() won't fix it (please see Attrs are never observed unless added with @attr decorator #208 for the details). When I look at the code of the latest version of Catalyst, I see no "connection" between initializeAttrs() and defineObservedAttributes(). The link is the attr() function...

I created those 2 issues hoping we could tackle them one by one, but now it seems to me they're more interdependent than I thought. Sorry about that 😅

Maybe this summary will make it easier:

attrs.js

// Using a word "instance" everywhere for simplicity

/**
 * Maps instances to their attributes
 */
const attrs = new WeakMap(...)

/**
 * Adds a new attribute to the mapping
 */
function attr(proto, key) {
	// The ONLY place where `attrs` is populated
}

/**
 * Defines computed properties on an instance with values from DOM
 */
function initializeAttrs(instance, names = []) {
	// Takes attribute names from the `names` argument OR from the `attrs` variable
}

/**
 * Defines a static computed `observedAttributes` property on an instance
 */
function defineObservedAttributes(classObject) {
	// Takes attribute names ONLY from the `attrs` variable
}

Please let me know if it's clear or not. I can also prepare an example repo or something 🙂

@keithamus
Copy link
Member

Thanks for the detailed reply @kristoforsalmin! We can work to fix the breakage, such that existing code continues to work (after all that’s the semver contract!).

We’re working on a 2.0 release which significantly changes how these fields are managed, especially for non-decorator code. It might be preferable to wait for this release - we hope to get a pre-release out this week. In 2.0, instead of using the decorator you can do the following:

import {controller, attr} from '@github/catalyst'
class AnExampleElement extends HTMLElement {
  static [attr.static] = ['open']
}
AnExampleElement = controller(AnExampleElement)

@kristoforsalmin
Copy link
Author

kristoforsalmin commented May 2, 2022

@keithamus cool! I'll just stick to whatever works now and wait for the upcoming release then 👍

Feel free to close those 2 issues (or keep them, if it makes sense). My main goal was to simply let you know about the behavior and my findings. Majority of people are probably using TypeScript anyway, so they won't even notice 😄

And thanks for making Catalyst! So far I only enjoyed its simple design and determination to fix "boilerplaty" aspects of Web Components with just a pinch of features on top and no more than that.

@keithamus keithamus added this to the v2 milestone May 3, 2022
@keithamus keithamus added the @attr label May 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants