Refactor Connection.js, transports, and Toolkit.js #42
Conversation
|
Why was a new PR opened instead of updating the existing one? |
|
The original PR contained more commits, updates to the tests and other files. I decided to exclude those commits from this PR and have those changes be in a separate PR next. Also these changes were added to separate branch therefore could not use the same PR. If its easier I can push these commits over to |
lib/Connection.js
Outdated
| options.username = username; | ||
| options.password = password; | ||
| options.transport = 'idb'; | ||
| options.returnError = false; |
There was a problem hiding this comment.
could do something like:
options = {
database: optionsOrDb,
username: username,
...
}There was a problem hiding this comment.
Will patch to be
options = {
returnError: false,
transport: 'idb',
transportOptions: {
database: optionsOrDb,
username,
password
}
}
lib/Connection.js
Outdated
| options = {}; | ||
| options.database = optionsOrDb; | ||
| options.username = username; | ||
| options.password = password; |
There was a problem hiding this comment.
are database, username, and password not transport options?
There was a problem hiding this comment.
Yes will add those properties to options.transportOptions sub object.
lib/Connection.js
Outdated
| delete options.transport; | ||
| delete options.returnError; | ||
|
|
||
| this.transportOptions = { ...options }; |
There was a problem hiding this comment.
Why is the user not passing in a transportOptions sub object?
There was a problem hiding this comment.
I was thinking the user would pass in the object in the format like:
{
returnError,
transport,
username,
password,
....
}We could design it for the user to pass in the object like:
{
returnError,
transport,
transportOptions: { username, password, ....}
}There was a problem hiding this comment.
I think that's best. Any transport-specific options are in the transportOptions key and you just pass it through to the transport. Then there's no confusion about what key is used for what.
Allow Connection.js Constuctor to accept a single object see #25 Allow Connection.run() function to return error as first param of callback see #2 Establish one code path in Connection Constructor - when pre v1.0 signature is used create an options object - add properties to this options object - this mimics the current v1.0 way of passing a signle object - that way constructor follows on code path Options object contain sub object transportOptions - This contains properties such as databse, username, etc - This sub object is passed to the transport Rename variables away from I_* prefix Rename getConnection() -> getTransportOptions() Rename cmd -> commandList Moved verbose to be a property of Connection - verbose is added to transportOptions during run() - this is because verbose can be adjusted with debug() - add print of xmlOutput also when in verbose mode
- We no longer support sync mode which had an option to set timeout
Update transports interface to accept config, xmlInput, done idb transport Use iPLUGR512k variant instead of iPLUG512k to iterate over a result set Ensure conn and stmt are always cleaned up properly - even during error states the stmt and conn objects are now cleaned - add function clean() to avoid duplication - this commit should help resolve issue #17 rest transport Validate required: database, username, password Enforce output buffer to be set to the max - before default was only set now output buffer is always set to the max Small fixes add mising semicolons, remove some comments, etc
- moved irest.js - moved istoredp.js
- Now all toolkit methods will have error first callbacks
I'm going to clean up a bit and squash incremental commits together to be more stream-lined |
|
Cleaned up and Squashed:
Into:
Going merge PR into v1.0-dev |
Connection class:
Constructor now supports passing a single object for configuration. see issue #25
Old signature with positional arguments are still supported.
reworked Connection.run() method:
run function invokes the transport function and passes:
run function now provides error first callback by default this resolved issue #2.
This can be configured through
returnErrorproperty.removed setTimeout() and timeout property since we no longer have sync mode, see issue #32
renamed internal variables away from I_* prefix that was used before.
No longer set defaults for transport options simply pass the options object to the transport.
Transports now deals with setting defaults and validate required parameters are OK.
renamed
getConnection()togetTransportOptions()Transports
updated to interface to accept config, xmlInput, done
In
istoredp.jsmoved to useiPLUGR512Kprocedure which returns a result set.transports were moved lib/transports/ directory
Toolkit.js
Updated Toolkit methods to return error first callbacks