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

Use defineJQueryPlugin method only in the base component #34114

Closed
wants to merge 1 commit into from

Conversation

rohit2sharma95
Copy link
Collaborator

Fixes #34103

There are multiple duplicated DOMContentLoaded events attached to the document, they are not really needed.

It is the very beginning of the idea, this approach may work IMO.
⚠️ Tests are not updated yet

/CC @twbs/js-review for the early feedback 🙂

@rohit2sharma95 rohit2sharma95 changed the title Use defineJQueryPlugin only in the base component Use defineJQueryPlugin method only in the base component May 26, 2021
@alpadev
Copy link
Contributor

alpadev commented May 27, 2021

I could be wrong but when used with a bundler there might be no global bootstrap object.

Another scenario that I could think of as problematic, is when the bundle is included after the document was loaded. Since you call defineJQueryPlugin() in the base-component module, the other components might not yet be defined, because the base-component is a dependency and needs to execute upfront, and onDOMContentLoaded will run the code synchronously.

@rohit2sharma95
Copy link
Collaborator Author

the other components might not be defined yet because the base-component is a dependency and needs to execute upfront and onDOMContentLoaded will run the code synchronously.

onDOMContentLoaded will run the code when the content has been loaded (It means the other plugins are also loaded). Is not it? 🤔

@alpadev
Copy link
Contributor

alpadev commented May 27, 2021

onDOMContentLoaded will run the code when the content has been loaded (It means the other plugins are also loaded). Is not it? 🤔

const onDOMContentLoaded = callback => {
  if (document.readyState === 'loading') {
    document.addEventListener('DOMContentLoaded', callback)
  } else {
    callback()
  }
}

When the script is already added while the page loads, the DOMContentLoaded event will trigger after the script executed. So yes.
But when the document was loaded already and the script is loaded/injected after, the callback is executed directly.
For example when loading the bundle asynchronously or by adding it dynamically with JS.

@alpadev
Copy link
Contributor

alpadev commented May 27, 2021

I mean, I'm talking about edge cases here. (You may read this for more details: https://javascript.info/onload-ondomcontentloaded#domcontentloaded-and-scripts - specially the warning block.)
The problem with the synchronous execution IMO can be fixed on our end by wrapping the callback within setTimeout for example.

The first problem might be a bigger one, because we can't control if someone uses a bundler like webpack that wouldn't expose bootstrap on window (without us doing this explicitly).

@rohit2sharma95
Copy link
Collaborator Author

Closing in favor of #34158

@alpadev
Copy link
Contributor

alpadev commented Jun 2, 2021

Oh I didn't mean to replace that PR, sorry @rohit2sharma95 :(

Figured you see some potential in refactoring our approach we add our components to jQuery (and as a side effect, also to get rid of the multiple listeners).

@rohit2sharma95 rohit2sharma95 deleted the rs-dom-content-loaded branch June 2, 2021 11:30
@rohit2sharma95
Copy link
Collaborator Author

NP @alpadev ❤️
Your PR was better, there is an issue related to it though #32664

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
2 participants