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

Refactor renderer.getClassList() into renderer.addClass(), renderer.removeClass() #2762

Open
caridy opened this issue Mar 24, 2022 · 5 comments

Comments

@caridy
Copy link
Contributor

caridy commented Mar 24, 2022

Function setScopeTokenClassIfNecessary calls getClassList(elm) on an element, and assumes the shape of the result value to be a DOM Object and calls .add() on it. Probably we should have a renderer api for just the add operation itself in one go.

@nolanlawson nolanlawson changed the title invalid dot notation on objects produced by renderer Refactor renderer.getClassList() into renderer.addClass(), renderer.removeClass() Aug 8, 2022
@git2gus
Copy link

git2gus bot commented Aug 8, 2022

This issue has been linked to a new work item: W-11568519

@nolanlawson
Copy link
Contributor

Rather than exposing the entire classList:

getClassList: (element: E) => DOMTokenList;

... the renderer should have hooks to add or remove classes.

@nolanlawson
Copy link
Contributor

I'm not sure this is entirely possible, because of this code:

get classList(): DOMTokenList {
const vm = getAssociatedVM(this);
const {
elm,
renderer: { getClassList },
} = vm;
if (process.env.NODE_ENV !== 'production') {
// TODO [#1290]: this still fails in dev but works in production, eventually, we should
// just throw in all modes
assert.isFalse(
isBeingConstructed(vm),
`Failed to construct ${vm}: The result must not have attributes. Adding or tampering with classname in constructor is not allowed in a web component, use connectedCallback() instead.`
);
}
return getClassList(elm);
},

We need to expose this.classList inside of a LightningElement, and to do that we need to have a getClassList on the renderer that returns the entire DOMTokenList.

@caridy
Copy link
Contributor Author

caridy commented Aug 23, 2022

Sure, but that doesn't mean we use that value to interact with, we just pass it along... then we can have new APIs just for the diffing. Basically, we have two separate buckets, APIs used for rendering, and APIs used for the element instance.

@nolanlawson
Copy link
Contributor

@caridy What is the benefit in that case? We would have the following APIs on the renderer:

  • getClassList
  • addClass
  • removeClass
  • containsClass
  • classListLength

(This is based on the APIs we actually use in the engine.)

If we expose getClassList anyway, it's not clear to me why we should have the other APIs as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants