Skip to content
This repository has been archived by the owner on Jun 1, 2022. It is now read-only.

webpack-typescript: DOMContentLoaded triggered twice #564

Closed
Thanood opened this issue Jul 1, 2016 · 17 comments
Closed

webpack-typescript: DOMContentLoaded triggered twice #564

Thanood opened this issue Jul 1, 2016 · 17 comments
Labels

Comments

@Thanood
Copy link
Contributor

Thanood commented Jul 1, 2016

Hi,

one of our users found a problem with the document.ready event triggered a second time and after some components have been attached. See the issue here for full context and console logs: aurelia-ui-toolkits/aurelia-materialize-bridge#200

In the above mentioned issue, we're using Materialize which initializes itself on $(document).ready() or the DOMContentLoaded event. Because this is triggered twice, Materialize is initialized twice which leads to a couple of problems with its controls.

This only happens with the webpack skeleton. If you do the same in a "non-webpack" skeleton it behaves like expected. The DOMContentLoaded event is fired only once.

I've uploaded a reproduction here: https://github.com/Thanood/webpack-domready-issue
There are two skeletons in that repo: typescript and typescript-webpack. Both contain the same modifications:

  • a "DOMContentLoaded" listener in index html, logging to the console
  • a "DOMContentLoaded" listener in an imported file which logs to the console (this is to simulate any library which relies on this DOM event to initialize itself)
  • a custom attribute with an attached() handler, logging to the console

This is the log that's being generated when using the webpack version:

document.ready
DEBUG [aurelia] Loading plugin aurelia-templating-binding.
DEBUG [aurelia] Configured plugin aurelia-templating-binding.
DEBUG [aurelia] Loading plugin aurelia-templating-resources.
DEBUG [aurelia] Configured plugin aurelia-templating-resources.
DEBUG [aurelia] Loading plugin aurelia-history-browser.
DEBUG [aurelia] Configured plugin aurelia-history-browser.
DEBUG [aurelia] Loading plugin aurelia-templating-router.
DEBUG [aurelia] Configured plugin aurelia-templating-router.
DEBUG [aurelia] Loading plugin aurelia-event-aggregator.
DEBUG [aurelia] Configured plugin aurelia-event-aggregator.
INFO [aurelia] Aurelia Started
DEBUG [templating] importing resources for app.html Array[3]
DEBUG [templating] importing resources for nav-bar.html Array[0]
DEBUG [templating] importing resources for welcome.html Array[0]
custom-attr.attached
document.ready import

With the non-webpack version the last line document.ready import is missing, as expected. 😄

Ping @JoschaMetze who found this.

@EisenbergEffect
Copy link
Contributor

@niieani Can you take a look at this?
@Thanood Do you know what is causing the issue? Any recommendations?

@niieani
Copy link
Contributor

niieani commented Jul 11, 2016

Odd. I'll add it to my ToDo list.

@Thanood
Copy link
Contributor Author

Thanood commented Jul 11, 2016

Thanks for looking into it.
Unfortunately my webpack experience is limited. So no idea what happens.

@niieani
Copy link
Contributor

niieani commented Jul 25, 2016

This shouldn't be happening in the 1.0.0 anymore.
I cannot reproduce with the recent changes to how Webpack is handled. Please test this after we release 1.0.0 and if the problem is still there, let me know.

@niieani niieani closed this as completed Jul 25, 2016
@Thanood
Copy link
Contributor Author

Thanood commented Jul 28, 2016

I've just tried this with Aurelia 1.0 but an old skeleton. The problem is still there.
I'll use the 1.0 skeleton and try again.

@Thanood
Copy link
Contributor Author

Thanood commented Jul 28, 2016

Doesn't happen with the 1.0.0 skeleton. 👍
Thanks!

btw. it would be nice if "compileOnSave": false was set in tsconfig.json.

@JoschaMetze
Copy link

Unfortunately it still is an issue in my opinion, but maybe @Thanood has a different approach.
It is correct that using DOMContentLoaded doesn't throw the import message, but using $( document ).ready still does. @Thanood try to add the following in dom-ready.ts:
$( document ).ready(function() { console.log('document.ready import'); });

@niieani
Copy link
Contributor

niieani commented Jul 28, 2016

@JoschaMetze what do you mean by saying:

doesn't throw the import message

@Thanood compileOnSave is by default false, not sure why would we need it there explicitly? Unless it's set to true -- that I don't know about?

@JoschaMetze
Copy link

Sorry for not stating that more clearly, I meant that the DOMContentLoaded event isn't triggered twice (as was the original implementation from @Thanood, see the last line of his log), so the bug is resolved for the DOMContentLoaded event. But unfortunately not for the document.ready()-event which is used quite regularly in materialize-css.
Or to rephrase, the problem in the original repro case has been resolved, but is still there using the jquery-events/document.ready.

@niieani
Copy link
Contributor

niieani commented Jul 28, 2016

document.ready cannot fire twice, so instead my guess is that the code is imported twice in your case, and thus the handler is executed twice. This can be caused by materialize-css being listed in the entry point and then being required again in some package, causing the handler to be set twice. Generally, import side effects are not recommended (with ES2015 modules startup should always be explicit, e.g. wrapped in a function), so this also seems like a bug in the way materialize-css initializes itself.

@Thanood
Copy link
Contributor Author

Thanood commented Jul 28, 2016

I will have to try this but when this is true:

try to add the following in dom-ready.ts: $( document ).ready(function() { console.log('document.ready import'); });

then materialize-css is not even involved.

@JoschaMetze did you try with dropdown? Because there is a known issue about dropdown options since the recent Materialize release 0.97.7: Dogfalo/materialize#3417 (comment)

regarding tsconfig: sorry for being off-topic. I'll check if this is a bug in atom-typescript because mine compiles on save.

@Thanood
Copy link
Contributor Author

Thanood commented Jul 28, 2016

Well, actually it "fires" twice (the SO post has a different context), since it behaves like a promise - and that is because it uses a deferred.

That doesn't change much, though.

So I suppose either you have two materialize-css imports as @niieani stated or it's the dropdown issue I posted above.. Anyway, the case at hand here (DOM event triggered twice) is solved.

Let's continue trying to find a solution here:
aurelia-ui-toolkits/aurelia-materialize-bridge#200

Thanks.. 😄

@niieani
Copy link
Contributor

niieani commented Jul 28, 2016

It's a deferred, but the passed in method should only fire once if it was set only once.
When you add it to dom-ready.ts like that, you still set it as a side effect of the import evaluation.

@Thanood
Copy link
Contributor Author

Thanood commented Jul 28, 2016

I may be wrong...
But why does this log twice, then?

$( document ).ready(function() { console.log('document.ready 1'); });
$( document ).ready(function() { console.log('document.ready 2'); });

@niieani
Copy link
Contributor

niieani commented Jul 28, 2016

@Thanood
Copy link
Contributor Author

Thanood commented Jul 29, 2016

Of course not and it certainly is not a good way of doing this (in a module env), but it simulates how materialize works. There may be other libraries, afaik even Bootstrap attaches event handlers that way.

But that's not the point, really. I just wanted to show (or more verify) that $(document).ready launches whatever you pass in even after the DOMContentLoaded event has fired. That does not mean that DOMContentLoaded is triggered twice but it's still possible for Materialize to initialize itself twice when you import it twice.

Let me make this clear: Listening to DOMContentLoaded yourself behaves correctly now.

I think you are right. Either the library is imported twice or @JoschaMetze is running into that dropdown issue. At least that is the current state of this problem. If I'm wrong and there is another issue with Aurelia/webpack, I'll report back. But for now I suppose it's related to imports or Materialize bugs.

@JoschaMetze
Copy link

Just for reference, I hit the dropdown issue, as @Thanood assumed correctly. The version from materialize-css@master works. Thanks for your help @niieani.

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

No branches or pull requests

4 participants