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

PreactBaseElement: Support returning a function for "selector" prop defs #33935

Merged
merged 8 commits into from Apr 30, 2021

Conversation

caroqliu
Copy link
Contributor

This PR adds support for an as configuration in component prop definitions to be paired with definitions that provide a selector and single: true. single: false does not support as as we do not have a use case for this currently.

Follow up for #33636 and #33105 to provide a generic soln from PreactBaseElement. This also addresses the arrow function issue discussed here.

* @param {!Object|undefined} props
* @return {!PreactDef.VNode}
*/
function SlotWithProps(props) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This actually has the same issue mentioned in the other thread. Here, SlotWithProps function is being recreated every time we render, which means the Component identity changes every time. This will cause the <slot> DOM node to be removed, a new one created, and then inserted during every diff.

Now that we're operating on the PreactBaseElement level, we can actually cache this per the element instance. Eg:

const cache: WeakMap<Element, {oldDefauls: (!Object|undefined), component: Component}> = new WeakMap();

export function createSlot(element, name, defaultProps, as) {
  element.setAttribute('slot', name);
  if (!as) {
    return <Slot {...(defaultProps || EMPTY)} name={name} />;
  }

  const cached = cache.get(element);
  if (cached && shallowEquals(cached.oldProps, defaultProps)) {
    return cached.component;
  }
  function SlotWithProps(props) {
    return <Slot {...(defaultProps || EMPTY)} name={name} {...props} />;
  }
  cache.set(element, { oldProps: defaultProps, component: SlotWithProps });

  return SlotWithProps;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for clarifying! Updated.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. I'd like to see a unit test verifying the <slot> DOM node is the same after a rerender, and possibly one asserting that it is new after the defaultProps changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To get the second case, we will have to enable both deep prop def checking in shouldMutationBeRerendered and include subtree: true in CHILDREN_MUTATION_INIT -- wanted to confirm this is OK.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can avoid it with:

PBE['props'] = {
  next: {
    selector: '…',
    props: {
      bar: {attr: 'bar'},
    },
  },
  foo: {
    attr: 'foo',
  },
};

// Mutate slot prop, but this won't trigger a rerender
el.querySelector('next').setAttribute('bar', 'other');

// Mutate an observed attr to trigger rerender
el.setAttribute('foo', 'foooey');

If it's more complicated than that, I think it's ok with just the unit tests you wrote.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh nice, that works. I removed the changes to the mutation observer then. Thanks!

* @param {!Object|undefined} props
* @return {!PreactDef.VNode}
*/
function SlotWithProps(props) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. I'd like to see a unit test verifying the <slot> DOM node is the same after a rerender, and possibly one asserting that it is new after the defaultProps changes.

test/unit/preact/test-slot.js Outdated Show resolved Hide resolved
* @param {!Object|undefined} props
* @return {!PreactDef.VNode}
*/
function SlotWithProps(props) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can avoid it with:

PBE['props'] = {
  next: {
    selector: '…',
    props: {
      bar: {attr: 'bar'},
    },
  },
  foo: {
    attr: 'foo',
  },
};

// Mutate slot prop, but this won't trigger a rerender
el.querySelector('next').setAttribute('bar', 'other');

// Mutate an observed attr to trigger rerender
el.setAttribute('foo', 'foooey');

If it's more complicated than that, I think it's ok with just the unit tests you wrote.

caroqliu and others added 2 commits April 29, 2021 16:45
Co-authored-by: Justin Ridgewell <justin@ridgewell.name>
@caroqliu caroqliu merged commit 81ae745 into ampproject:main Apr 30, 2021
rochapablo pushed a commit to rochapablo/amphtml that referenced this pull request Aug 30, 2021
…efs (ampproject#33935)

* Support "as" with "selector" prop defs

* Cache per element instance

* Add unit tests

* Update test/unit/preact/test-slot.js

Co-authored-by: Justin Ridgewell <justin@ridgewell.name>

* Do not observe subtree mutations

* (empty)

Co-authored-by: Justin Ridgewell <justin@ridgewell.name>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants