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

Support window.onload #2643

Merged
merged 6 commits into from Jul 16, 2019

Conversation

@kt3k
Copy link
Contributor

commented Jul 13, 2019

This PR tries to solve #2386. I added triggerLoadEvent function for triggering window.onload function (I put it under denoMain namespace to avoid leaking to global namespace). run_script function (in //cli/mian.rs) calls it after the main_module executed.


closes #2386

window.onload = function(e): void {
console.log(`got ${e.type} event`);
};
console.log("main");

This comment has been minimized.

Copy link
@bartlomieju

bartlomieju Jul 13, 2019

Contributor

Might be good idea to add 1-2 imports in this file, that do console.log to ensure that onload is actually called after imports are resolved. WDYT?

This comment has been minimized.

Copy link
@kt3k

kt3k Jul 14, 2019

Author Contributor

Agree. I added an import statement.

cli/main.rs Outdated
@@ -314,6 +314,7 @@ fn run_script(flags: DenoFlags, argv: Vec<String>) {
worker
.execute_mod_async(&main_module, false)
.and_then(move |()| {
js_check(worker.execute("denoMain.triggerLoadEvent()"));

This comment has been minimized.

Copy link
@kevinkassimo

kevinkassimo Jul 13, 2019

Contributor

I feel we might want to give it a more general name (I am thinking that we might add more lifecycle hooks at some point)

This comment has been minimized.

Copy link
@kt3k

kt3k Jul 14, 2019

Author Contributor

Changed to denoMain.lifecycles.onLoad for now. Maybe we can add more lifecycles in this namespace. I'm open to any further suggestion!

js/main.ts Outdated
@@ -51,3 +52,5 @@ export default function denoMain(name?: string): void {
replLoop();
}
}

denoMain.triggerLoadEvent = triggerLoadEvent;

This comment has been minimized.

Copy link
@kevinkassimo

kevinkassimo Jul 13, 2019

Contributor

I think we need to at some point collect all private calls (e.g. window.workerMain, window.xevalMain) and this to a centralized location to avoid confusing users with other public ones. Maybe putting into window._$$deno so that they look like window._$$deno.triggerLoadEvent? (debating about the naming)

This comment has been minimized.

Copy link
@kt3k

kt3k Jul 14, 2019

Author Contributor

I was a little uncomfortable about using denoMain as a namespace. I agree that we should have something like what you suggest.

@kt3k kt3k force-pushed the kt3k:feature/onload branch from 69cd61e to 86474e4 Jul 14, 2019

@ry

This comment has been minimized.

Copy link
Collaborator

commented Jul 14, 2019

@kt3k Nice!

In the browser you can also do something like window.addEventListener("load", x). I think that would be nice to support here.
https://developer.mozilla.org/en-US/docs/Web/API/Window/load_event

@kt3k

This comment has been minimized.

Copy link
Contributor Author

commented Jul 15, 2019

@ry OK. I'll try that. Probably we can reuse the EventTarget implementation.

@kt3k kt3k force-pushed the kt3k:feature/onload branch from 2267c77 to f323ba9 Jul 15, 2019

onload(e);
}
});
*/

This comment has been minimized.

Copy link
@ry

ry Jul 15, 2019

Collaborator

(remove before landing)

Show resolved Hide resolved js/event_target.ts
"load",
(e: Event): void => {
console.log(`got ${e.type} event in event handler (main)`);
}

This comment has been minimized.

Copy link
@ry

ry Jul 15, 2019

Collaborator

:D

@kt3k kt3k force-pushed the kt3k:feature/onload branch from f323ba9 to 26d6a89 Jul 15, 2019

@kt3k

This comment has been minimized.

Copy link
Contributor Author

commented Jul 15, 2019

I changed the several things since the initial version ( diff: 26d6a89 )

  • Made window an EventTarget
  • Removed //js/lifecycle.ts and onLoad function, instead registered the event listener to window which invokes window.onload
  • Dispatches load event after main_module loaded
  • Refactored EventTarget to prevent leaking its properties as global variable when making window an EventTarget.
@ry

This comment has been minimized.

Copy link
Collaborator

commented Jul 15, 2019

@kt3k Looks great!
Would you mind adding an example to the website/manual.md showing the usage of window.onload ?

Maybe we should switch the examples that use a "main" function to use onload instead? https://gist.github.com/ry/49f21bc8e56c25fd45fd0aa44b17d531

@kt3k kt3k force-pushed the kt3k:feature/onload branch from 8baa075 to 1d25513 Jul 16, 2019

@ry

ry approved these changes Jul 16, 2019

Copy link
Collaborator

left a comment

LGTM - this is a great feature !

@ry ry merged commit 9c45499 into denoland:master Jul 16, 2019

3 checks passed

Travis CI - Pull Request Build Passed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
license/cla Contributor License Agreement is signed.
Details

@kt3k kt3k deleted the kt3k:feature/onload branch Jul 16, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
4 participants
You can’t perform that action at this time.