Added support for Promises in the variable system #3554

eahefnawy · 2017-05-03T12:42:20Z

What did you implement:

Added support for async sources in the variable system, beginning with the file source. Your JS files can now perform async operations and return a promise that would be resolved by the variable system.

How did you implement it:

Previously, the variable system was completely sync, so I had to control the flow of the entire system and it's methods and make it promise-ready. All methods now return a promise to assure everything is running in the correct flow.

I've also ditched the traverse module since it doesn't work smoothly with promises and instead I'm using a lodash based implementation for object traversal and updates.

How can we verify it:

I've tested 17 different cases for the variable systems that covers pretty much everything the variables system can do, including the new promises in files support. Here's the config I used, but feel free to get crazy creative:

# serverless.yml

service: serverless-async-vars

provider:
  name: aws
  runtime: nodejs6.10

custom:
  data: # some data to reference
    zero: 0
    nooo: false
  varA: ${env:TESTING} # env vars
  varB: ${opt:stage} # opts vars
  varC: ${self:provider.name} # self vars
  varD: ${env:TESTING}-${opt:stage} # two vars in a string
  varE: ${opt:${env:TESTING}hoo} # nested vars
  varF: ${opt:region, opt:stage} # overwrite functionality
  varG: ${file(./vars.js):hello} # JS file running sync
  varH: ${file(./vars.js):promised} # JS file running async/promised NEW!!!
  varI: ${file(./vars.json):hoo.hoo2} # deep JSON file
  varJ: ${file(./vars.yml):hee.hee2} # deep YAML file
  varK: my stage is ${opt:stage} # vars sub string
  varL: your account number is ${opt:number} # number vars as sub string
  varM: ${opt:number} # preserving data type
  varN: ${self:plugins, self:package, self:service} # multiple overwrites when empty object and undefined
  varO: ${self:custom.data.zero, self:service} # shouldn't overwrite 0 values
  varP: ${self:custom.data.nooo, self:service} # shouldn't overwrite false values
  varQ: ${self:} # referencing the entire config file

functions:
  hello:
    handler: handler.hello

You'll need the TESTING env var to be set, and also the following vars files in your service root dir:
vars.js

module.exports.hello = () => {
  // Sync code
  return {nested: 'world'};
}

module.exports.promised = () => {
  // Async code
  return Promise.resolve('world');
}

vars.yml

hee:
  hee2: haa

vars.json

{
  "hoo": {
    "hoo2": "hum"
  }
}

You'll need to run the following command against this service for full test coverage:

sls package --stage dev --yahoo hi --region us-east-1 --number 1234

Todos:

Write tests
Write documentation
Fix linting errors
Make sure code coverage hasn't dropped
Provide verification config / commands / resources
Enable "Allow edits from maintainers" for this PR
Update the messages below

Is this ready for review?: YES
Is it a breaking change?: NO

jthomerson · 2017-05-03T13:01:11Z

Sweet @eahefnawy! Thanks! This will be a huge help for anyone who wants to load dynamic config from, for instance, an S3 bucket, a DynamoDB table, an API call, etc, etc!

pmuens

This is a really exciting PR! 💯

Here are a few comments / questions:

Why are native Promises used instead of Bluebird promises (since we use then throughout the whole codebase?)
Could you update the tests so that sinon-bluebird is used (introduced in add sinon-bluebird to make promises in tests a bit cleaner #3000). This way the tests are in sync
Shoud we maybe remove the traverse package in this PR? AFAIK there are no other places where traverse is used.

Also tested it locally. So far everything works fine. Will test it more thoroughly the upcoming days!

Below is the serverless.yml file I've used for testing.

I exported the TESTING=ya env variable and ran serverless package --stage dev --yahoo hi --region us-east-1 --number 1234.

To validate it you could look into the Outputs section in the updated CloudFormation file

service: async-vars

provider:
  name: aws
  runtime: nodejs6.10

custom:
  data: # some data to reference
    zero: 0
    nooo: false
  varA: ${env:TESTING} # env vars
  varB: ${opt:stage} # opts vars
  varC: ${self:provider.name} # self vars
  varD: ${env:TESTING}-${opt:stage} # two vars in a string
  varE: ${opt:${env:TESTING}hoo} # nested vars
  varF: ${opt:region, opt:stage} # overwrite functionality
  varG: ${file(./vars.js):hello} # JS file running sync
  varH: ${file(./vars.js):promised} # JS file running async/promised NEW!!!
  varI: ${file(./vars.json):hoo.hoo2} # deep JSON file
  varJ: ${file(./vars.yml):hee.hee2} # deep YAML file
  varK: my stage is ${opt:stage} # vars sub string
  varL: your account number is ${opt:number} # number vars as sub string
  varM: ${opt:number} # preserving data type
  varN: ${self:plugins, self:package, self:service} # multiple overwrites when empty object and undefined
  varO: ${self:custom.data.zero, self:service} # shouldn't overwrite 0 values
  varP: ${self:custom.data.nooo, self:service} # shouldn't overwrite false values
  # varQ: ${self:} # referencing the entire config file

functions:
  hello:
    handler: handler.hello

resources:
  Outputs:
    varA:
      Value: ${self:custom.varA}
      Export:
        Name: varA
    varB:
      Value: ${self:custom.varB}
      Export:
        Name: varB
    varC:
      Value: ${self:custom.varC}
      Export:
        Name: varC
    varD:
      Value: ${self:custom.varD}
      Export:
        Name: varD
    varE:
      Value: ${self:custom.varE}
      Export:
        Name: varE
    varF:
      Value: ${self:custom.varF}
      Export:
        Name: varF
    varG:
      Value: ${self:custom.varG}
      Export:
        Name: varG
    varH:
      Value: ${self:custom.varH}
      Export:
        Name: varH
    varI:
      Value: ${self:custom.varI}
      Export:
        Name: varI
    varJ:
      Value: ${self:custom.varJ}
      Export:
        Name: varJ
    varK:
      Value: ${self:custom.varK}
      Export:
        Name: varK
    varL:
      Value: ${self:custom.varL}
      Export:
        Name: varL
    varM:
      Value: ${self:custom.varM}
      Export:
        Name: varM
    varN:
      Value: ${self:custom.varN}
      Export:
        Name: varN
    varO:
      Value: ${self:custom.varO}
      Export:
        Name: varO
    varP:
      Value: ${self:custom.varP}
      Export:
        Name: varP
    # varQ:
    #   Value: ${self:custom.varQ}
    #   Export:
    #     Name: varQ

pmuens · 2017-05-04T11:57:58Z

lib/classes/Variables.js

      }
    });

-    this.service.provider.variableSyntax = variableSyntaxProperty;
-    return this.service;
+    return Promise.all(populateAll).then(() => {


Is there any reason why native Promises are used vs. Bluebird promises? (AFAIK Bluebird promises are faster. Not sure if this hold true for Node.js).

pmuens · 2017-05-04T12:01:24Z

lib/classes/Variables.test.js

@@ -42,12 +42,12 @@ describe('Variables', () => {
      const serverless = new Serverless();

      const populatePropertyStub = sinon
-        .stub(serverless.variables, 'populateProperty');
+        .stub(serverless.variables, 'populateProperty').returns(Promise.resolve());


You could do .resolves() since we're now using the sinon-bluebird package (introduced in #3000)

pmuens · 2017-05-04T12:04:51Z

package.json

@@ -96,6 +96,7 @@
    "js-yaml": "^3.6.1",
    "json-refs": "^2.1.5",
    "lodash": "^4.13.1",
+    "lodash-deep": "^2.0.0",


Having this we could npm uninstall the traverse package, right?

ryanmurakami · 2017-05-04T16:58:31Z

docs/providers/aws/guide/variables.md

@@ -164,6 +164,25 @@ module.exports.schedule = () => {
   };
 }
 ```
+If your use case requires handling with dynamic/async data sources (ie. DynamoDB, API calls...etc), you can also return a promise that would be resolved as the value of the variable:


uppercase 'Promise'

pmuens

Seems like some old Promise references are still there.

Will do a follow-up PR and clean things up 👍

pmuens · 2017-05-05T09:25:28Z

lib/classes/Variables.js

      if (typeof property === 'string') {
-        property = that.populateProperty(property, true);
-        t.update(property);
+        const populateSingleProperty = new Promise((resolve) => this


Shouldn't this be new BbPromise()?

pmuens · 2017-05-05T09:26:00Z

lib/classes/Variables.js

-          valueToPopulate = this.overwrite(variableString);
-        } else {
-          valueToPopulate = this.getValueFromSource(variableString);
+        const singleValueToPopulate = new Promise((resolve) => {


Shouldn't this be new BbPromise()?

pmuens · 2017-05-05T09:27:40Z

lib/classes/Variables.test.js

-      serverless.variables.populateVariable.restore();
+        serverless.variables.overwrite.restore();
+        serverless.variables.populateVariable.restore();
+        return Promise.resolve();


return BbPromise.resolve()

pmuens · 2017-05-05T09:27:58Z

lib/classes/Variables.test.js

-      serverless.variables.populateVariable.restore();
+        serverless.variables.getValueFromSource.restore();
+        serverless.variables.populateVariable.restore();
+        return Promise.resolve();


return BbPromise.resolve()

pmuens · 2017-05-05T09:42:47Z

package.json

@@ -104,7 +105,6 @@
    "semver": "^5.0.3",
    "semver-regex": "^1.0.0",
    "shelljs": "^0.6.0",
-    "traverse": "^0.6.6",


This needs some shrinkwrapping,

See serverless/serverless#3554

eahefnawy added 2 commits May 2, 2017 23:54

supported promises in the variable system

7cadf28

renamed promises

4810719

eahefnawy mentioned this pull request May 3, 2017

Support Promises in variables referencing javascript ( ${file(file.js):value} ) #3073

Closed

pmuens added this to the 1.13 milestone May 3, 2017

pmuens added the stage/in-review label May 3, 2017

pmuens self-requested a review May 3, 2017 12:57

fixed variables tests

bd33c98

pmuens reviewed May 4, 2017

View reviewed changes

ryanmurakami reviewed May 4, 2017

View reviewed changes

fixed all unit tests

9b10da6

eahefnawy merged commit 358ea69 into master May 4, 2017

eahefnawy deleted the async-vars branch May 4, 2017 18:00

pmuens reviewed May 5, 2017

View reviewed changes

This was referenced May 5, 2017

Follow up on async vars #3560

Closed

Custom variableSyntax bug is not detected in new async variable system #3561

Closed

k1LoW added a commit to faultline/faultline that referenced this pull request May 12, 2017

Use Promise variable instead of local plugin

f467a9c

See serverless/serverless#3554

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added support for Promises in the variable system #3554

Added support for Promises in the variable system #3554

eahefnawy commented May 3, 2017 •

edited

jthomerson commented May 3, 2017

pmuens left a comment •

edited

pmuens May 4, 2017

pmuens May 4, 2017

eahefnawy May 4, 2017

pmuens May 4, 2017

eahefnawy May 4, 2017

ryanmurakami May 4, 2017

pmuens left a comment

pmuens May 5, 2017

pmuens May 5, 2017

pmuens May 5, 2017

pmuens May 5, 2017

pmuens May 5, 2017

Added support for Promises in the variable system #3554

Added support for Promises in the variable system #3554

Conversation

eahefnawy commented May 3, 2017 • edited

What did you implement:

How did you implement it:

How can we verify it:

Todos:

jthomerson commented May 3, 2017

pmuens left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pmuens left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eahefnawy commented May 3, 2017 •

edited

pmuens left a comment •

edited