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

Register only one DOMContentLoaded event listener in onDOMContentLoaded #34158

Merged
merged 3 commits into from
Jun 22, 2021

Conversation

alpadev
Copy link
Contributor

@alpadev alpadev commented Jun 1, 2021

Closes #34103

Since #34114 is more focused around changing the way we register our components with jQuery, this PR is changing the onDOMContentLoaded utility function, to make use of an array to cache callbacks (while the document is loading) and execute them all at once. Therefore we register only one DOMContentLoaded event listener instead of one for every call to the function.

@GeoSot
Copy link
Member

GeoSot commented Jun 1, 2021

Seems fine by me.

(You may could add a test)

@GeoSot GeoSot self-requested a review June 1, 2021 12:18
@alpadev alpadev force-pushed the alpa/refactor-ondomcontentloaded branch from 6760d0a to 7278a2b Compare June 2, 2021 07:06
@alpadev
Copy link
Contributor Author

alpadev commented Jun 2, 2021

@GeoSot Changed the implementation so it wouldn't add an event listener when the function isn't called during loading state. Updated the test to check that addEventListener isn't called more than once.

@GeoSot
Copy link
Member

GeoSot commented Jun 2, 2021

@rohit2sharma95 give it a look, if you like, for a second opinion

Copy link
Collaborator

@rohit2sharma95 rohit2sharma95 left a comment

Choose a reason for hiding this comment

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

Good work @alpadev 🥇

js/tests/unit/util/index.spec.js Outdated Show resolved Hide resolved
@alpadev alpadev force-pushed the alpa/refactor-ondomcontentloaded branch from 7278a2b to 9c9bb7c Compare June 2, 2021 09:51
@alpadev alpadev requested a review from XhmikosR June 3, 2021 22:17
…aded function

Instead of adding an event listener everytime the utility function is called, cache the callbacks and execute them all at once.
@alpadev alpadev force-pushed the alpa/refactor-ondomcontentloaded branch from d4dd813 to 99e448e Compare June 3, 2021 23:04
@alpadev
Copy link
Contributor Author

alpadev commented Jun 3, 2021

@XhmikosR removed the IIFE as requested

@XhmikosR XhmikosR changed the title Register only one DOMContentLoaded event listener in onDOMContentLoaded Register only one DOMContentLoaded event listener in onDOMContentLoaded Jun 22, 2021
@XhmikosR XhmikosR merged commit 4927388 into main Jun 22, 2021
@XhmikosR XhmikosR deleted the alpa/refactor-ondomcontentloaded branch June 22, 2021 17:19
marvin-hinkley-vortx pushed a commit to Vortx-Inc/bootstrap that referenced this pull request Aug 18, 2021
…oaded` (twbs#34158)

* refactor: reuse one DOMContentLoaded event listener in onDOMContentLoaded function

Instead of adding an event listener everytime the utility function is called, cache the callbacks and execute them all at once.

* refactor: drop iife for onDOMContentLoaded

Co-authored-by: XhmikosR <xhmikosr@gmail.com>
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.

Remove duplicate DOMContentLoaded events
5 participants