[APITEST-766] Added Packages to Collection Schema #1352
Conversation
lib/collection/script.js
Outdated
|
|
||
| /** | ||
| * @arguments {Script.prototype} | ||
| * @type {Array<Object>} |
There was a problem hiding this comment.
This would provide more info about the interface:
| * @type {Array<Object>} | |
| * @type {Array<{ id: string, name: string }>} |
lib/collection/script.js
Outdated
| @@ -62,6 +62,12 @@ _.assign(Script.prototype, /** @lends Script.prototype */ { | |||
| * @type {string} | |||
| */ | |||
| this.type = options.type || 'text/javascript'; | |||
|
|
|||
| /** | |||
| * @arguments {Script.prototype} | |||
There was a problem hiding this comment.
Typo?
| * @arguments {Script.prototype} | |
| * @augments {Script.prototype} |
There was a problem hiding this comment.
Do we even need this JSDoc tag? 🤔
lib/collection/script.js
Outdated
| * @arguments {Script.prototype} | ||
| * @type {Array<Object>} | ||
| */ | ||
| this.packages = options.packages || []; |
| @@ -62,6 +63,9 @@ _.assign(Script.prototype, /** @lends Script.prototype */ { | |||
| * @type {string} | |||
| */ | |||
| this.type = options.type || 'text/javascript'; | |||
|
|
|||
| this.packages = options.packages || {}; | |||
There was a problem hiding this comment.
Let's keep the @type properties for packages. Needs to be done for line 52 as well.
| @@ -25,7 +25,8 @@ describe('Collection', function () { | |||
| script: { | |||
| id: 'my-script-1', | |||
| type: 'text/javascript', | |||
| exec: ['console.log("This doesn\'t matter");'] | |||
| exec: ['console.log("This doesn\'t matter");'], | |||
| packages: {} | |||
There was a problem hiding this comment.
Why do we need to add the empty key everywhere? It's supposed to be optional, no?
There was a problem hiding this comment.
Yep yep, just needed to add this to the assertions after conversion since we're defaulting to an empty object, no need for this here.
| @@ -10,7 +10,8 @@ describe('EventList', function () { | |||
| id: 'my-global-script-1', | |||
| script: { | |||
| type: 'text/javascript', | |||
| exec: 'console.log("hello");' | |||
| exec: 'console.log("hello");', | |||
| packages: [{ id: 'script-package-1', name: 'package1' }] | |||
aditya-baradwaj
force-pushed
the
feature/APITEST-766-Add-Packages-to-collection-schema
branch
from
99457c4
to
805b014
Compare
| @@ -62,6 +63,9 @@ _.assign(Script.prototype, /** @lends Script.prototype */ { | |||
| * @type {string} | |||
| */ | |||
| this.type = options.type || 'text/javascript'; | |||
|
|
|||
| this.packages = options.packages || {}; | |||
There was a problem hiding this comment.
Do we need to add a default value if this key is not present? If it's an optional property, I'm guessing... no?
There was a problem hiding this comment.
@coditva Do we mean to make this completely optional?
For consistency purposes, I thought it would be best to have this property in all event objects, and rather than leaving it as undefined in the cases where it isn't available, it seemed better to have it as an empty object.
We can change this in such a way that we move this under an
if (options.packages) to make this completely optional, but it does not seem ideal IMO. Do let me know if this is not the case.
It would be completely optional from a User perspective in either way tho.
There was a problem hiding this comment.
A whole lot of collections will start having empty packages field. I want to avoid it for the collections that don't even use it.
There was a problem hiding this comment.
makes sense, I'll make it so that this.packages is only set if options.packages exists. Sounds good?
This PR adds the concept of packages to the collection schema and the postman-collection entity.
In the events object, we have a Listen (String) and Script Object to encapsulate all the data related to a script.
Inside the Script Object, we have the following -
We are adding a fourth entity - Packages
Packages is an array of objects, each of which will contain the name of the package and the ID of the package. This enables us to correctly resolve the packages needed for the execution of the particular parent event.
The events object in the collection will now look something like this -
Implementation documents -