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

Why did startStackFunction get renamed to startStackFn from 8.x to 10.x #27671

Closed
Raynos opened this issue May 13, 2019 · 7 comments
Closed

Why did startStackFunction get renamed to startStackFn from 8.x to 10.x #27671

Raynos opened this issue May 13, 2019 · 7 comments
Labels
assert Issues and PRs related to the assert subsystem.

Comments

@Raynos
Copy link
Contributor

Raynos commented May 13, 2019

In assert.AssertionError, for some reason options.startStackFunction got renamed to options.startStackFn from v8.x to v10.x

This means you have to do var e = new AssertionError({ startStackFn: fn, startStackFunction: fn }) to have code that works across both versions of nodejs.

This seems like an unnecessary breaking change :( is there no policy or code review for API stability ?

@BridgeAR
Copy link
Member

@Raynos that was a mistake on my side. I did not anticipate that the error class would actually really be used in the wild (it was not documented) and that this option would be used. I'll open a PR to accept both as valid options but that will probably not help a lot anymore.

BridgeAR added a commit to BridgeAR/node that referenced this issue May 13, 2019
This makes sure the `AssertionError` still accepts the
`stackStartFunction` option as alternative to the `stackStartFn`.

Fixes: nodejs#27671
@BridgeAR
Copy link
Member

I'll try to backport this as well.

@BridgeAR BridgeAR added the assert Issues and PRs related to the assert subsystem. label May 13, 2019
@Raynos
Copy link
Contributor Author

Raynos commented May 14, 2019

@BridgeAR you are my hero.

Here is my use case for context.

'use strict'

const process = require('process')
const { AssertionError } = require('assert')

module.exports = nextTickAssert

function nextTickAssert (value, message) {
  let errorToBeThrown

  if (!value) {
    let generatedMessage = false

    if (arguments.length === 0) {
      generatedMessage = true
      message = 'No value argument passed to `assert.ok()`'
    } else if (message == null) {
      generatedMessage = true
      message = String(value) + ' == true'
    }

    if (message instanceof Error) {
      errorToBeThrown = message
    } else {
      const err = new AssertionError({
        message: message,
        actual: value,
        expected: true,
        operator: '==',
        stackStartFunction: nextTickAssert,
        stackStartFn: nextTickAssert
      })
      err.generatedMessage = generatedMessage
      errorToBeThrown = err
    }

    process.nextTick(() => {
      throw errorToBeThrown
    })
  }
}

I want to use assert() but somehow bail outside of async/await; I want sanity assertions that should never happen and fail my program.

I'll open an issue / PR on node core for this seperately as it's a useful issue that came up on twitter a while ago.

@BridgeAR
Copy link
Member

@Raynos I believe you actually want to use the --unhandled-rejections=strict mode in that case. That is not yet available in Node.js < 11.x but it should be backported to at least 10.x.

@BridgeAR
Copy link
Member

An easier solution would also be the following:

'use strict'

const process = require('process')
const assert = require('assert').strict

module.exports = nextTickAssert

function nextTickAssert (value, message) {
  try {
    assert(value, message)
  } catch (err) {
    Error.captureStackTrace(err, nextTickAssert);
    process.nextTick(() => {
      throw err
    })
  }
}

@Raynos
Copy link
Contributor Author

Raynos commented May 14, 2019

Your solution means i don't have an issue with the AssertionError constructor anymore. I didn't think about catching the error ...

I'd have to write some benchmarks though. The existing assert() calls are mostly free because they are a single if branch on a boolean.

@BridgeAR
Copy link
Member

@Raynos the cost should be identical in newer Node.js versions. I believe Node.js 8 is already fine with try / catch statements but I forgot in what version the performance cliff got removed.

There is one significant difference between these two versions though: the error message will be different (in case no message argument was passed through). Due to the abstraction, assert is not going to be able to determine the actual call site anymore but you said you want this only as safe guard that should actually never trigger.

targos pushed a commit that referenced this issue May 17, 2019
This makes sure the `AssertionError` still accepts the
`stackStartFunction` option as alternative to the `stackStartFn`.

PR-URL: #27672
Fixes: #27671
Reviewed-By: Yongsheng Zhang <zyszys98@gmail.com>
Reviewed-By: Anto Aravinth <anto.aravinth.cse@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
assert Issues and PRs related to the assert subsystem.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants