Skip to content

Conversation

@grouma
Copy link
Member

@grouma grouma commented Sep 15, 2017

Exposing a way for tests to send a load exception to the test server. This will be used by DDC tests here: https://github.com/dart-lang/sdk/blob/0edc00171a9881d330b633cf0f43b0654e2653c1/pkg/dev_compiler/lib/js/legacy/dart_library.js#L16

lib/dart.js Outdated
}

// Listen for loadException events and forward to the server.
window.addEventListener('loadException', function(e) {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: consider something a bit more specific like dartTestLoadException or similar, since this is on the global window object you may run into conflicts

Copy link
Member Author

Choose a reason for hiding this comment

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

Changed to dartLoadException given that this logic can reside outside of tests.

@grouma grouma merged commit f7df911 into dart-lang:master Sep 15, 2017
whesse pushed a commit to dart-lang/sdk that referenced this pull request Sep 18, 2017
Dispatch load exception so that others can react without capturing the corresponding error. Will be used in conjunction with dart-lang/test#682

Bug:
Change-Id: I19458ac01ba013cac6ad5408b3b763777e50ca4e
Reviewed-on: https://dart-review.googlesource.com/6644
Reviewed-by: Jake Macdonald <jakemac@google.com>
Commit-Queue: Gary Roumanis <grouma@google.com>
@@ -1,3 +1,7 @@
## 0.12.24+5

* Expose a way for tests to forward a `loadException` to the server.
Copy link
Member

Choose a reason for hiding this comment

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

If we're exposing a new feature, this should bump the minor version to 0.12.25. It should also describe how tests can forward load exceptions both here and in either the README or the API docs.

If it's not a user-visible feature, the CHANGELOG should just say "no user-visible changes".

Copy link
Member Author

Choose a reason for hiding this comment

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

Noted. This is a non user-visible feature.

}

// Listen for dartLoadException events and forward to the server.
window.addEventListener('dartLoadException', function(e) {
Copy link
Member

Choose a reason for hiding this comment

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

Why do we have our own event type for this, as opposed to catching zone exceptions or handling global JS errors?

Copy link
Member Author

Choose a reason for hiding this comment

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

Global JS errors can happen for a number of reasons and may not be indicative of an actual load exception. What we really want is if DDC failed to load. The above event is passed by the DDC's dart_library helper. See https://dart-review.googlesource.com/c/sdk/+/6644

Copy link
Member

Choose a reason for hiding this comment

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

It would be good to add a comment indicating that dartLoadException is an exception produced by DDC in particular.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants