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

Audit assert usages #4012

Merged
merged 1 commit into from
Dec 13, 2019
Merged

Audit assert usages #4012

merged 1 commit into from
Dec 13, 2019

Conversation

Pessimistress
Copy link
Collaborator

Generally speaking, assert should only be used for sanity checking user-provided parameters and be helpful in debugging.

Change List

  • Remove assert on parameters passed between internal code
  • Remove assert where it's impossible to fail (e.g. a crash would happen before the assertion is reached)
  • Avoid assert in performance critical functions

@coveralls
Copy link

Coverage Status

Coverage decreased (-2.4%) to 80.66% when pulling 487f6cf on x/assert into 742cff9 on master.

Copy link
Contributor

@tsherif tsherif left a comment

Choose a reason for hiding this comment

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

Great idea. I think I'll do a similar pass in luma.

Copy link
Collaborator

@ibgreen ibgreen left a comment

Choose a reason for hiding this comment

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

A cleanup was certainly in order.

I have has this idea about asserts:

  • we could offer a mechanism to have asserts be removed in production. E.e probe.gl can provide a babel-plugin-remove-assert (a simplification of, or extension of, the log removal plugin that was just added to probe.gl).
  • The problem is how apps would apply this plugin - select from multiple library builds, or have their app build systems explicitly apply this plugin on all modules that match /\.gl/?. But such a plugin would remove a bunch of concerns around overhead, especially around bundle size impact of the assert message strings.

@@ -193,13 +193,10 @@ export default class Layer extends Component {
// Always unprojects to the viewport's coordinate system
unproject(xy) {
const {viewport} = this.context;
assert(Array.isArray(xy));
Copy link
Collaborator

Choose a reason for hiding this comment

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

Many of these asserts can be replaced by simple static type checking. For instance, without even adopting typescript, running the typescript checker and just letting it parse the existing code and jsdoc comment headers, can catch a lot of incorrect parameter passing cases. It does require some massaging of existing code to make it work, I have a proof-of-concept PR in math.gl for that.

@Pessimistress
Copy link
Collaborator Author

probe.gl can provide a babel-plugin-remove-assert

It's not that easy to do it from the application configuration. Depending on which dist env the module is resolved to, transpiled code can look quite different from the source, e.g. layer.js in es5:

_log["default"].deprecated('layer.unprojectFlat')();

geojson.js in es5:

_core.log.assert(geojson.type, 'GeoJSON does not have type');

The app can of course set custom babel rules for the vis.gl dependencies, although the setup would likely be cumbersome and fragile. It is also very difficult to verify that the plugin was indeed effective, without digging into the minified bundle.

If we provide multiple library builds, as you commented before, we wouldn't want to use conditional require, which breaks tree-shaking. We will instead offer the production builds without a valid entry point in package.json, and the user will need to specify their own module resolution rules in their bundle settings.

@Pessimistress Pessimistress merged commit e64aab1 into master Dec 13, 2019
@Pessimistress Pessimistress deleted the x/assert branch December 13, 2019 17:02
tgorkin added a commit that referenced this pull request Dec 17, 2019
…gatsby-examples

* commit '515466ce583f1419d2b0199daa31f850d3627bb8': (76 commits)
  fix website build (#4031)
  Tweak to scenegraph layer fix (#4027)
  Fix s2 layer polygon generation (#4024)
  Smooth edges in scatterplot (#4021)
  pydeck: Update for new @deck.gl/json API and add additional tes… (#4020)
  Fix ScenegraphLayer asset wait (#4025)
  v8.0-beta.1 Changelog (#4022)
  Bump dependency versions in modules, examples and docs (#4015)
  v8.0.0-beta.1
  bump loaders.gl to beta.5 (#4018)
  v8.0 beta
  pydeck: Make a single bundle for use in standalone and Jupyter rendering (#4010)
  Clean up LayerManager (#4011)
  Audit assert usages (#4012)
  React: eventManager listens to all children (#4013)
  Fix plot example labels (#4014)
  Bump loaders.gl to 2.0.0-beta (#4009)
  TextLayer: support background color (#3903)
  v8.0 what's new (#4001)
  Improve createProps perf (#4007)
  ...
chrisgervang pushed a commit that referenced this pull request Jan 31, 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.

4 participants