Netflix / x-element

A dead simple starting point for custom elements.
Apache License 2.0
28 stars 12 forks source link

Removes all “nudges” from “x-element”. #82

Closed theengineear closed 3 years ago

theengineear commented 3 years ago

There’s been a lot of discussion about the opinions “x-element” makes to be helpful — and sometimes the accusation is that “x-element” is getting in the way.

This PR removes all such nudges. This should feel inline with our choice to remove errors for attempting to access internal properties on the host (e.g., host.myInternalProp just returns undefined now).

In particular, we no longer error for the following:

And, we no longer warn for the following:

Closes #77.

theengineear commented 3 years ago

@klebba , @charricknflx , @codeStryke , @erahhal — here's a controversial PR to bike shed on. We added these errors/warnings to try and be helpful. However, it seems like they might actually just be getting in the way. It's no sweat to me if we don't have them any more, but I wanted to know what ya'll think.

One thing we might do is add some lines of documentation that explains why you should familiarize with the "global" attributes, "data-" attributes, "aria-" attributes, and global properties as defined in the W3C specifications.

For example, you're not longer going to get warnings about interesting edge cases like the class attribute and it's associated className — however, my guess is that all of us are already intimately familiar and it's not the goal of "x-element" to teach us such nuance.

codeStryke commented 3 years ago

I would still like to argue that we change the framework defaults to any one of the following

AND leave the nudges in place.

klebba commented 3 years ago

@theengineear what is the path forward here? This one fell off my radar but I would be glad to help push it over the finish line. It also sounds like @codeStryke has objections to the removal of nudges & has other requested changes (probably need to open a separate issue to discuss)

theengineear commented 3 years ago

Yah! This issue did seem to fall off. My current opinion is to remove all the nudges as proposed here. And to be clear all the nudges we're talking about are really things that are outside the purview of XElement — e.g., "data-*" attributes, etc.

The goal of XElement is not to warn you if you're misusing the platform. We only need to warn you if you're misusing the XElement interface.

As for the attributes/properties behavior discussion. There's already an issue for that — checkout #80. Let's have that conversation there and only discuss nudges here.

codeStryke commented 3 years ago

It's fine you can remove the nudges, no more opposition from me

theengineear commented 3 years ago

@klebba — I think we're all good to merge on this one. If anything, it might be good to take one more look at the top-level description to make sure you're OK with the change. You're welcome to take a pass at the code itself, mostly we just remove a bunch of construction-time and run-time checks 👌

klebba commented 3 years ago

@theengineear Thanks for keeping me in the loop! Your succinct phrasing here really captures the intent of the change:

The goal of XElement is not to warn you if you're misusing the platform. We only need to warn you if you're misusing the XElement interface.

Can you clarify this statement, I'm not sure I follow:

If an attribute is set for an unserializable property — we no longer observe such attributes at all.

I gave the change set a once over; all in favor of removing run-time checks and slimming down element boot is sweet too. ~10% line reduction — great work!

theengineear commented 3 years ago

Can you clarify this statement, I'm not sure I follow:

If an attribute is set for an unserializable property — we no longer observe such attributes at all.

Previously, we would observe attributes for properties which were not serializable. We did this solely for the purpose to erroring if you tried to interact with the attribute. Now, instead of erroring, we just do nothing.

// Before.
element.setAttribute('non-serializable-property'); // Error: Unexpected deserialization...

// After.
element.setAttribute('non-serializable-property'); // No more error, nothing really happens at all.
klebba commented 3 years ago

@theengineear changes look great, no apprehension about merging here, was only trying to understand code-level changes to satisfy my own curiosity. Nice work!