Remove goog.asserts.* #5619

ahocevar · 2016-07-19T20:27:51Z

This pull requests replaces type check hint assertions with type casts, library sanity check assertions with conditional console.assert statements in debug mode, and runtime sanity checks with assertions that throw an ol.AssertionError with an error code for lookup outside the library.

See #5600.

There is one remaining goog.asserts.assertInstanceof, for which there is a separate pull request (#5616).

Fixes #5619.

probins · 2016-07-20T07:07:27Z

src/ol/ol.js

+
+  /**
+   * Error code. The meaning of the code can be found on
+   * {@link http://openlayers.org/en/lates/errors.html} (replace `latest` with


/lates/latest/

probins · 2016-07-20T07:34:16Z

src/ol/feature.js

    } else {
-      goog.asserts.assertInstanceof(obj, ol.style.Style,
-          'obj should be an ol.style.Style');
+      ol.assert(obj instanceof ol.style.Style, 41);


you might want to add the error as a comment here too

probins · 2016-07-20T07:48:04Z

ISTM there's 2 types of runtime errors, which need to be handled differently. Most of them catch application errors; in this case, the developer should see the error when testing the app, and correct the problem. But some of them catch data errors, like problems with vector files. These may be user-supplied and beyond the developer's control, so the developer has to catch these, and inform the user. It might be worth having a different error type or process for these, but in any case developers need to consider how to handle them. I think this has to be documented and mentioned in the release notes.

ahocevar · 2016-07-20T07:58:58Z

@probins The lines between the two are blurry, so I decided to not introduce two error types. Think of someone reading in WMC context files or some other user provided map configurations. In this case, configuration errors can occur in a context that is beyond the app developer's influence.

But I agree that this change in general needs to be mentioned in the release notes. I'm going to add that.

probins · 2016-07-20T09:47:24Z

ok. I'd also document the console.asserts. Something like 'There are additional console assertion checks when in debug mode (when the new ol.DEBUG flag is true). As this is true by default, it's recommended that those creating custom builds set this to false so these assertions are stripped.' I'd also document this in ol.js where ol.DEBUG is defined.

ahocevar · 2016-07-20T12:54:05Z

Good suggestion. Done.

tschaub · 2016-08-03T07:00:24Z

src/ol/ol.js

+   */
+  this.message = 'Assertion failed. See ' +
+      (ol.VERSION ? 'http://openlayers.org/en/' + ol.VERSION.split('-')[0] : '') +
+      '/doc/errors.html#' + code + ' for details.';


URL snob comment: how about openlayers.org/en/latest/errors/#123 instead (adding doc/errors/index.md instead of doc/errors.md?

Sounds reasonable. I'll make that change. Thanks for the suggestion 😄.

tschaub · 2016-08-04T04:11:17Z

changelog/upgrade-notes.md


+#### Changes in the way assertions are handled
+
+Previously, minified builds of the library did not have any assertions. This caused applications to fail silently or with cryptic stack traces. Starting with this release, developers get notified of many runtime errors through the new `ol.AssertionError`. This error has a `code` property. The meaning of the code can be found on http://openlayers.org/en/latest/doc/errors/. There are additional console assertion checks when in debug mode when the new `ol.DEBUG` compiler flag (previously `goog.DEBUG`) is `true`. As this is `true` by default, it is recommended that those creating custom builds set this to `false` so these assertions are stripped.'


There is still a lot of goog.DEBUG in the library (and our build configs). Would it make sense to switch to ol.DEBUG when the rest of goog is gone? With this branch, I assume a build config would have to set both to false to remove debug code.

tschaub · 2016-08-04T05:37:04Z

Wow. Thanks for taking this on @ahocevar.

What do you think about leaving goog.DEBUG until the rest is gone?

ahocevar · 2016-08-04T09:03:12Z

@tschaub I have renamed ol.DEBUG to goog.DEBUG, and removed gratuitous asserts like the one you identified.

This pull requests replaces type check hint assertions with type casts, library sanity check assertions with conditional console.assert statements in debug mode, and runtime sanity checks with assertions that throw an ol.AssertionError with an error code for lookup outside the library.

tschaub · 2016-08-04T21:00:47Z

Very nice. Merge it!

ahocevar · 2016-08-04T21:02:34Z

Thanks for the review and the good suggestions @tschaub.

probins · 2016-08-04T21:16:21Z

👏 removing the 'gratuitous' asserts is a considerable simplification. That's the biggest outstanding part of the Closure library. Just remains a few bits and pieces ...

probins · 2016-08-05T06:51:54Z

this creates a whole lot of extra goog.DEBUGs. I'm not convinced that anybody actually uses the existing goog.DEBUGs, but wouldn't want to let that get in the way of removing goog. So the obvious way forward is to add ol.DEBUG as suggested here at first, change the release notes, and then simply find/replace goog.DEBUG with ol.DEBUG. I should be able to find time for this either today or over the weekend, or is this something you want to do @ahocevar? Anything else that needs to be considered?

ahocevar · 2016-08-05T09:44:39Z

@probins Thanks for willing to take this on! Before renaming goog.DEBUG to ol.DEBUG, we should remove the few remaining Closure library bits (goog.dom and a few other minor ones I think).

ahocevar force-pushed the remove-goog-asserts branch from d5b4f91 to 629a52c Compare July 19, 2016 20:52

probins reviewed Jul 20, 2016
View reviewed changes

ahocevar force-pushed the remove-goog-asserts branch from 629a52c to 3987b68 Compare July 20, 2016 07:23

probins reviewed Jul 20, 2016
View reviewed changes

ahocevar force-pushed the remove-goog-asserts branch from 3987b68 to 4c355d9 Compare July 20, 2016 07:56

ahocevar force-pushed the remove-goog-asserts branch 2 times, most recently from 083d386 to f0d3183 Compare July 20, 2016 08:23

ahocevar force-pushed the remove-goog-asserts branch from f0d3183 to d920e4d Compare July 20, 2016 12:53

ahocevar force-pushed the remove-goog-asserts branch 2 times, most recently from d28314d to a60c282 Compare July 21, 2016 08:11

ahocevar mentioned this pull request Aug 2, 2016

Assertion error is thrown in ol.debug when ol.map logo is set to false #5671

Closed

tschaub reviewed Aug 3, 2016
View reviewed changes

ahocevar force-pushed the remove-goog-asserts branch from a60c282 to b8b93af Compare August 3, 2016 07:38

tschaub reviewed Aug 4, 2016
View reviewed changes

ahocevar force-pushed the remove-goog-asserts branch from 5960d7d to 2fdd8a3 Compare August 4, 2016 09:06

ahocevar added 5 commits August 4, 2016 11:29

Use a nicer URL for the error descriptions

55ab570

Use goog.DEBUG instead of ol.DEBUG for now

e0015b3

Remove gratuitous debug assertions

e18d41b

Remove asserts from ol.format.WFS

bf275b0

ahocevar force-pushed the remove-goog-asserts branch from 2fdd8a3 to bf275b0 Compare August 4, 2016 09:43

ahocevar merged commit b25ce40 into openlayers:master Aug 4, 2016

ahocevar deleted the remove-goog-asserts branch August 4, 2016 21:02

tschaub mentioned this pull request Aug 7, 2016

Re-enable ESLint config #5697

Merged


		#### Changes in the way assertions are handled

		Previously, minified builds of the library did not have any assertions. This caused applications to fail silently or with cryptic stack traces. Starting with this release, developers get notified of many runtime errors through the new `ol.AssertionError`. This error has a `code` property. The meaning of the code can be found on http://openlayers.org/en/latest/doc/errors/. There are additional console assertion checks when in debug mode when the new `ol.DEBUG` compiler flag (previously `goog.DEBUG`) is `true`. As this is `true` by default, it is recommended that those creating custom builds set this to `false` so these assertions are stripped.'

Uh oh!

Remove goog.asserts.* #5619

Remove goog.asserts.* #5619

Uh oh!

Conversation

ahocevar commented Jul 19, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

probins Jul 20, 2016

Choose a reason for hiding this comment

Uh oh!

probins Jul 20, 2016

Choose a reason for hiding this comment

Uh oh!

probins commented Jul 20, 2016

Uh oh!

ahocevar commented Jul 20, 2016

Uh oh!

probins commented Jul 20, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ahocevar commented Jul 20, 2016

Uh oh!

tschaub Aug 3, 2016

Choose a reason for hiding this comment

Uh oh!

ahocevar Aug 3, 2016

Choose a reason for hiding this comment

Uh oh!

tschaub Aug 4, 2016

Choose a reason for hiding this comment

Uh oh!

tschaub commented Aug 4, 2016

Uh oh!

ahocevar commented Aug 4, 2016

Uh oh!

tschaub commented Aug 4, 2016

Uh oh!

ahocevar commented Aug 4, 2016

Uh oh!

probins commented Aug 4, 2016

Uh oh!

probins commented Aug 5, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ahocevar commented Aug 5, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ahocevar commented Jul 19, 2016 •

edited

Loading

probins commented Jul 20, 2016 •

edited

Loading

probins commented Aug 5, 2016 •

edited

Loading