Skip to content

Fix listenForBind bugs#50

Closed
keithamus wants to merge 8 commits intomasterfrom
fix-listenforbind-allow-any-node-to-be-given-as-first-param
Closed

Fix listenForBind bugs#50
keithamus wants to merge 8 commits intomasterfrom
fix-listenforbind-allow-any-node-to-be-given-as-first-param

Conversation

@keithamus
Copy link
Copy Markdown
Contributor

@keithamus keithamus commented Jul 1, 2020

This fixes two bugs in listenForBind:

  1. The type of el was inferred, so it becomes Document | undefined which is not what we want. The type should be Node to allow us to register other, non document nodes, should we need to.
  2. The MutationObserver only listened for addedNodes, checking to see if they have the data-action attribute, however, if the node inserted has children we should also bind those, so a test + code has been added to do this.

I've refactored the code quite extensively, the new code is much simpler - but its worth looking at each commit to see how it has changed.

@keithamus keithamus requested a review from a team as a code owner July 1, 2020 13:24
Copy link
Copy Markdown

@mr-mig mr-mig left a comment

Choose a reason for hiding this comment

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

🥇

@keithamus keithamus force-pushed the fix-listenforbind-allow-any-node-to-be-given-as-first-param branch from 2aed261 to bc7b2cd Compare July 1, 2020 17:24
Comment thread src/bind.ts
if (node.hasAttribute('data-action')) {
queue.add(node)
}
queue.add(node)
Copy link
Copy Markdown
Contributor

@muan muan Jul 17, 2020

Choose a reason for hiding this comment

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

The subtle differences between bind and processQueue got me a bit confused so I created a test case at bc7b2cd...1f61c3d.

It looks like this line gets called when a new Catalyst element gets injected as well, so are bound repeatedly through both

  1. connectedCallback -> bind -> bindActionToController
  2. listenForBind -> this line -> processQueue -> bindActionsToController

Copy link
Copy Markdown
Contributor

@muan muan Jul 17, 2020

Choose a reason for hiding this comment

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

Here's a test case: 7d40085

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Real nice catch! Thanks for this @muan!

@keithamus
Copy link
Copy Markdown
Contributor Author

Superseded by #53

@keithamus keithamus closed this Jul 20, 2020
@keithamus keithamus deleted the fix-listenforbind-allow-any-node-to-be-given-as-first-param branch July 21, 2020 14:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants