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

1.0.0 #1

Closed
wants to merge 30 commits into from
Closed

1.0.0 #1

wants to merge 30 commits into from

Conversation

loay
Copy link
Contributor

@loay loay commented Apr 18, 2016

See the proposal in strongloop/loopback#1650 (comment) for details.

Connect to strongloop/loopback#1650

node_js:
- "0.10"
- "0.12"
- "iojs"
Copy link
Member

Choose a reason for hiding this comment

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

iojs is no longer relevant, please add "4" (LTS) and "5" (stable) instead

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@bajtos
Copy link
Member

bajtos commented Apr 19, 2016

@loay your coding style seems a bit inconsistent to me, some lines have trailing semi colon, some don't. Please add eslint with eslint-config-loopback and fix any issues reported by linter.

/* istanbul ignore next */
var defer = typeof setImmediate === 'function'
? setImmediate
: function(fn){ process.nextTick(fn.bind.apply(fn, arguments)) }
Copy link
Member

Choose a reason for hiding this comment

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

Huh, why is this needed? setImmediate is always a function in Node.js 0.10 and newer.

@bajtos
Copy link
Member

bajtos commented Apr 19, 2016

Please use a different file name than test/test.js, one that describes more what is being tested. For example index.test.js or perhaps integration.test.js

@bajtos bajtos removed their assignment Apr 19, 2016
var util = require('util');
var loopback = require('loopback');

describe('strongErrorHandler() when debug is set to true', function() {
Copy link
Member

Choose a reason for hiding this comment

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

Uh, maintaining two copies of test cases that are mostly identical is a nightmare. It's difficult to tell what is specific for the case when debug==true and what's same for both.

Ideally, the tests should be written in such way, that tests for things like nosniff header that do not depend on debug will use the default value and should be written in such way that they pass regardless of what's the default value. Then there should be tests focused solely on debug:false, debug:true and debug:undefined.

Copy link
Member

Choose a reason for hiding this comment

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

If you want to run the same set of tests with different config options, then create parameterised test suite instead:

var SCENARIOS = [
  { debug: false }, 
  { debug: true }
];

SCENARIOS.forEach(function(sc) {
  describe('strongErrorHandler with ' + JSON.stringify(sc), function() {
    // run the tests and use `sc.debug` for the debug option passed to `createServer`
  }
}

However, I think that's an overkill here.

@bajtos bajtos assigned loay and unassigned bajtos May 5, 2016
// json
} else if (type === 'json') {
var error = { message: err.message, stack: err.stack };
for (var prop in err) error[prop] = err[prop];
Copy link
Member

Choose a reason for hiding this comment

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

This does not follow the spec, please rework and add unit-tests too.

strongloop/loopback#1650 (comment)

JSON response fields

  1. debug:true - all error fields are sent in the HTTP response
  2. debug:false & status code is 4xx - the error object contains the following fields:
    • name
    • message - as provided by Error.message
    • statusCode
    • details
    • + any fields from options.safeFields
  3. debug:false & status code is 5xx or <400 or falsey - the error object
    contains the following fields
    • statusCode
    • message - the string from HTTP spec
    • + any fields from options.safeFields

Copy link
Member

Choose a reason for hiding this comment

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

Also note that the rules outlined above applies to other content types too. I.e. the message should be always the string from HTTP spec when statusCode is not 4xx, regardless whether we render HTML, JSON or text.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added the 3 conditions in JSON

@loay
Copy link
Contributor Author

loay commented May 6, 2016

@bajtos
I am missing:

  • cache the html template
  • clean up the test cases.

@loay loay assigned bajtos and unassigned loay May 6, 2016
@loay
Copy link
Contributor Author

loay commented May 6, 2016

@bajtos
PTAL

This was referenced May 11, 2016
@bajtos
Copy link
Member

bajtos commented May 11, 2016

@loay I am afraid there is still too many missing pieces to address before this can be landed. I have reworked some parts of this patch in #9 and created new issues to keep track of the remaining work.

@bajtos bajtos closed this May 11, 2016
@bajtos bajtos deleted the 1.0.0 branch August 10, 2016 11:29
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