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

Cleanup Event and EventTarget #4707

Merged
merged 2 commits into from
Apr 11, 2020
Merged

Conversation

kitsonk
Copy link
Contributor

@kitsonk kitsonk commented Apr 11, 2020

This PR does a couple things:

  • Refactors Event and EventTarget so that they better encapsulate their non-public data as well as are more forward compatible with things like DOM Nodes.
  • Moves dom_types.ts -> dom_types.d.ts which was always the intention, it was a legacy of when we used to build the types from the code and the limitations of the compiler. There was a lot of cruft in dom_types which shouldn't have been there, and mis-alignment to the DOM standards. This generally has been eliminated, though we still have some minor differences from the DOM (like the removal of some deprecated methods/properties).
  • Adds DOMException. Strictly it shouldn't inherit from Error, but most browsers provide a stack trace when one is thrown, so the behaviour in Deno actually better matches the browser.

Event still doesn't log to console like it does in the browser. I wanted to get this raised and that could be an enhancement later on (it currently doesn't either).

@@ -23,37 +23,30 @@ export type HeadersInit =
| Headers
Copy link
Member

Choose a reason for hiding this comment

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

This file contains a ton of overlap with cli/js/lib.deno.shared_globals.d.ts is there some way this code can be shared?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let me think about it. It is a valid point, and it is slowly killing us by a thousand papercuts.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I found a way... For things that are part of this PR (Event, EventTarget, CustomEvent, and DOMException) I have eliminated them from dom_types and instead am just using lib.deno.shared_globals.d.ts. I would recommending doing the rest as a subsequent PR. It does take a little bit of effort to make sure you get it right. Take a look at the last commit in isolation to get an idea of what I had to do.

Copy link
Member

@ry ry left a comment

Choose a reason for hiding this comment

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

LGTM - great clean up! I'm really glad you found a way to share types with lib.deno.shared_globals.d.ts. It seems there are a lot of other types that we can now eliminate from cli/js/web/dom_types.d.ts, but I agree this should be done later.

@ry ry merged commit fc4819e into denoland:master Apr 11, 2020
@ry ry mentioned this pull request Apr 11, 2020
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.

None yet

2 participants