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

Added support for circular references in the variable system #4144

Merged
merged 23 commits into from Dec 11, 2017

Conversation

Projects
None yet
6 participants
@eahefnawy
Member

eahefnawy commented Aug 23, 2017

What did you implement:

Closes #3627
Closes #3740
Closes #3711
Closes #3882

Added support for resolving circular dependencies in the variable system

How did you implement it:

I'm keeping track of all the nodes that has been resolved along with their values into a map so that the variable system doesn't have to take the same walk twice (end of the circle)

How can we verify it:

Try to reproduce issue #3627 , but to make sure nothing has been broken, use the following setup that includes a comprehensive coverage of the variable system edge cases, including the new circular support (varQ and varR).

  • serverless.yml
service:
  name: s1

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!!!
  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, NEW!
  varR: ${self:custom.varQ.custom.varQ.provider.name} # circular, NEW!

provider:
  name: aws
  runtime: nodejs6.10

functions:
  f1:
    handler: handler.hello
  • 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"
  }
}
  • export TESTING=ya
  • sls package --stage dev --yahoo hi --region us-east-1 --number 1234

After running this package command, you should see the resolved service in the console (I left out a console log for verification that I'll remove later)

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

@eahefnawy eahefnawy added this to the 1.21 milestone Aug 23, 2017

@pmuens pmuens self-requested a review Aug 23, 2017

@erikerikson erikerikson referenced this pull request Aug 23, 2017

Merged

Add Cyclic Object Serialization Support #4146

4 of 7 tasks complete

@pmuens pmuens removed the stage/in-review label Aug 24, 2017

@pmuens

pmuens requested changes Aug 24, 2017 edited

Really nice fix! I can imagine that this was pretty mind-bending! ☁️ 👍

Biiiig +1 for the nice issue description! That makes it super easy to test this PR 💪

I just tested it today with the following setup (I modified your test service slightly so that I can take a look at the Outputs section of the compiled CloudFormation template):

service:
  name: s1

custom:
  data: # some data to reference
    zero: 0
    nooo: false
  nonCircular:
    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!!!
    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:custom.circular.varR.custom.circular.varR.provider.name} # circular, NEW!
  circular:
    varR: ${self:} # referencing the entire config file, NEW!

provider:
  name: aws
  runtime: nodejs6.10

functions:
  f1:
    handler: handler.hello

resources:
  Outputs:
    VarsTest:
      Value: ${self:custom.nonCircular}

After running export TESTING=ya && serverless package --stage dev --yahoo hi --region us-east-1 --number 1234 (same commands you've used in your PR description) I get the following Outputs:

{
  "VarsTest": {
      "Value": {
        "varA": "ya",
        "varB": "dev",
        "varC": "aws",
        "varD": "ya-dev",
        "varE": "hi",
        "varF": "us-east-1",
        "varG": {
          "nested": "world"
        },
        "varH": "world",
        "varI": "hum",
        "varJ": "haa",
        "varK": "my stage is dev",
        "varL": "your account number is 1234",
        "varM": 1234,
        "varN": "s1",
        "varO": 0,
        "varP": false,
        "varQ": "aws"
      }
    }
}

This looks great. However I'm not sure about varF and varN.

  • varF --> Shouldn't this be dev (Or does the second value only kick in if the first is not present?)
  • varN --> shouldn't this be nested in name? The result (the service name) is correct, but this might confuse users of the variable system. I assume that this has smth. todo with the service and serviceObject functionality behind the scenes which we've introduced a couple of releases ago.

Not sure if this fix caused those regressions or if they were introduces earlier (which might be the case) 🤔.

@eahefnawy

This comment has been minimized.

Member

eahefnawy commented Aug 24, 2017

varF is correct, it overwrites only when it's undefined. As for varN, this is I think due to the backward compatibly that we have with the implementation of serviceObject. If it's an issue, the fix would be Service class related though, so I wouldn't fit it in this PR.

@erikerikson

We ran into a bug with this branch. This is a pared down service (from hello-etail to reproduce the problem:

service: hello
provider:
  name: aws
  runtime: nodejs4.3
custom:
  stage: ${opt:stage, self:provider.stage, self:custom.private.stage}
  domainName: ${self:custom.private.domainName}
  dnsHostName:
    Fn::If: # e.g. hello-retail.biz or stage.hello-retail.biz
      - ProductionStage
      - ${self:custom.domainName}
      - ${self:custom.stage}.${self:custom.domainName}
  originId:
    Fn::Join: # e.g. S3-hello-retail.biz or S3-stage.hello-retail.biz
      - ''
      - - 'S3-'
        - ${self:custom.dnsHostName}
  private:
    domainName: DOMAIN_NAME
resources:
  Conditions:
     ProductionStage:
       Fn::Equals:
         - ${self:custom.stage}
         - prod
  Resources:
    # ===================== S3 =====================
    HelloRetailWebBucket:
      Type: AWS::S3::Bucket
      Properties:
        BucketName: ${self:custom.dnsHostName}
        WebsiteConfiguration:
          IndexDocument: index.html
          ErrorDocument: error.html

I haven't gone too deep on isolating the case or problem but it seems to be relevant that

@@ -96,7 +97,9 @@ class Variables {
} else {
singleValueToPopulate = this.getValueFromSource(variableString)
.then(valueToPopulate => {
if (typeof valueToPopulate === 'object') {
if (typeof valueToPopulate === 'object'
&& typeof this.populatedObjectReferences[variableString] === 'undefined') {

This comment has been minimized.

@erikerikson

erikerikson Aug 25, 2017

Member

perhaps:
!(variableString in this.populatedObjectReferences)
as the second clause?

@@ -96,7 +97,9 @@ class Variables {
} else {
singleValueToPopulate = this.getValueFromSource(variableString)
.then(valueToPopulate => {
if (typeof valueToPopulate === 'object') {
if (typeof valueToPopulate === 'object'

This comment has been minimized.

@erikerikson

erikerikson Aug 25, 2017

Member

populateObject doesn't appear to handle arrays (which have typeof === 'object')
could this be a potential risk?

@erikerikson

This comment has been minimized.

Member

erikerikson commented Aug 25, 2017

oh yeah. symptom of issue from comment, from cloudformation-template-update-stack.json:

    "HelloRetailWebBucket": {
      "Type": "AWS::S3::Bucket",
      "Properties": {
        "BucketName": {
          "Fn::If": [
            "ProductionStage",
            "${self:custom.domainName}",
            "${self:custom.stage}.${self:custom.domainName}"
          ]
        },
        "WebsiteConfiguration": {
          "IndexDocument": "index.html",
          "ErrorDocument": "error.html"
        }
      }
    }
@erikerikson

This comment has been minimized.

Member

erikerikson commented Aug 25, 2017

note the unreplaced "${self:custom.domainName}" and "${self:custom.stage}.${self:custom.domainName}"

@pmuens

Thanks for the review @erikerikson 👍

I just added some comments about the test 👍.

I agree with the service vs. serviceObject comment you wrote. We shouldn't introduce it in this PR.

Next up I'll try to reproduce @erikerikson problem 🔝


serverless.variables.loadVariableSyntax();

const populateObjectStub = sinon

This comment has been minimized.

@pmuens

pmuens Aug 25, 2017

Member

This setup should be moved into a beforeEach block.

expect(populateVariableStub.called).to.equal(true);
expect(newProperty).to.deep.equal(variableValuePopulated);

serverless.variables.populateObject.restore();

This comment has been minimized.

@pmuens

pmuens Aug 25, 2017

Member

Cleanup should be moved in an afterEach block so that the restore is still done even if this test fails.

return serverless.variables.populateProperty(property).then(newProperty => {
expect(populateObjectStub.called).to.equal(false);
expect(getValueFromSourceStub.called).to.equal(true);
expect(populateVariableStub.called).to.equal(true);

This comment has been minimized.

@pmuens

pmuens Aug 25, 2017

Member

Can we be more specific about the count here?


return serverless.variables.populateProperty(property).then(newProperty => {
expect(populateObjectStub.called).to.equal(false);
expect(getValueFromSourceStub.called).to.equal(true);

This comment has been minimized.

@pmuens

pmuens Aug 25, 2017

Member

Can we be more specific about the count here?

@pmuens

This comment has been minimized.

Member

pmuens commented Aug 25, 2017

Alright. So I was able to reproduce this with @erikerikson service. I figured out which part breaks the variable system:

service: hello
provider:
  name: aws
  runtime: nodejs4.3
custom:
  stage: ${opt:stage, self:provider.stage, self:custom.private.stage}
  domainName: ${self:custom.private.domainName}
  dnsHostName:
    Fn::If: # e.g. hello-retail.biz or stage.hello-retail.biz
      - ProductionStage
      - ${self:custom.domainName}
      - ${self:custom.stage}.${self:custom.domainName}
  ###################### UNCOMMENT THIS TO MAKE IT BREAK
  # originId:
  #   Fn::Join: # e.g. S3-hello-retail.biz or S3-stage.hello-retail.biz
  #     - ''
  #     - - 'S3-'
  #       - ${self:custom.dnsHostName}
  ######################
  private:
    domainName: DOMAIN_NAME
resources:
  Conditions:
     ProductionStage:
       Fn::Equals:
         - ${self:custom.stage}
         - prod
  Resources:
    HelloRetailWebBucket:
      Type: AWS::S3::Bucket
      Properties:
        BucketName: ${self:custom.dnsHostName}
        WebsiteConfiguration:
          IndexDocument: index.html
          ErrorDocument: error.html

It looks like processing the originId doesn't work.

@eahefnawy eahefnawy modified the milestones: 1.22, 1.21 Aug 30, 2017

@pmuens

This comment has been minimized.

Member

pmuens commented Aug 31, 2017

Just tested it again today. Looks like the problem @erikerikson described is still not solved.

here's a template how this can be reproduced (modified version of 🔝):

service: hello
provider:
  name: aws
  runtime: nodejs4.3
custom:
  stage: ${opt:stage, self:provider.stage, self:custom.private.stage}
  domainName: ${self:custom.private.domainName}
  dnsHostName:
    Fn::If: # e.g. hello-retail.biz or stage.hello-retail.biz
      - ProductionStage
      - ${self:custom.domainName}
      - ${self:custom.stage}.${self:custom.domainName}
  ###################### THIS MAKES IT BREAKING
  originId:
    Fn::Join: # e.g. S3-hello-retail.biz or S3-stage.hello-retail.biz
      - ''
      - - 'S3-'
        - ${self:custom.dnsHostName}
  #####################
  private:
    domainName: DOMAIN_NAME
resources:
  Conditions:
     ProductionStage:
       Fn::Equals:
         - ${self:custom.stage}
         - prod
  Resources:
    HelloRetailWebBucket:
      Type: AWS::S3::Bucket
      Properties:
        BucketName: ${self:custom.dnsHostName}
        WebsiteConfiguration:
          IndexDocument: index.html
          ErrorDocument: error.html

This will compute the following:

{
  "BucketName": {
    "Fn::If": [
      "ProductionStage",
      "${self:custom.domainName}",
      "${self:custom.stage}.${self:custom.domainName}"
    ]
  }
}
@pmuens

This comment has been minimized.

Member

pmuens commented Sep 1, 2017

@eahefnawy I just wrapped my head around the example which breaks and the values for the resolved variables. Here's my take on that:

Before variable population:

service: hello
provider:
  name: aws
  runtime: nodejs4.3
custom:
  stage: ${opt:stage, self:provider.stage, self:custom.private.stage}
  domainName: ${self:custom.private.domainName}
  dnsHostName:
    Fn::If: # e.g. hello-retail.biz or stage.hello-retail.biz
      - ProductionStage
      - ${self:custom.domainName}
      - ${self:custom.stage}.${self:custom.domainName}
  ###################### THIS MAKES IT BREAKING
  originId:
    Fn::Join: # e.g. S3-hello-retail.biz or S3-stage.hello-retail.biz
      - ''
      - - 'S3-'
        - ${self:custom.dnsHostName}
  #####################
  private:
    domainName: DOMAIN_NAME
resources:
  Conditions:
     ProductionStage:
       Fn::Equals:
         - ${self:custom.stage}
         - prod
  Resources:
    HelloRetailWebBucket:
      Type: AWS::S3::Bucket
      Properties:
        BucketName: ${self:custom.dnsHostName}
        WebsiteConfiguration:
          IndexDocument: index.html
          ErrorDocument: error.html

After variable population:

service: hello
provider:
  name: aws
  runtime: nodejs4.3
custom:
  stage: dev # assuming the stage is passed in via options
  domainName: DOMAIN_NAME
  dnsHostName:
    Fn::If: # e.g. hello-retail.biz or stage.hello-retail.biz
      - ProductionStage
      - DOMAIN_NAME
      - dev.DOMAIN_NAME
  originId:
    Fn::Join: # e.g. S3-hello-retail.biz or S3-stage.hello-retail.biz
      - ''
      - - 'S3-'
        - Fn::If:
            - ProductionStage
            - DOMAIN_NAME
            - dev.DOMAIN_NAME
  private:
    domainName: DOMAIN_NAME
resources:
  Conditions:
     ProductionStage:
       Fn::Equals:
         - dev
         - prod
  Resources:
    HelloRetailWebBucket:
      Type: AWS::S3::Bucket
      Properties:
        BucketName:
          Fn::If: # e.g. hello-retail.biz or stage.hello-retail.biz
            - ProductionStage
            - DOMAIN_NAME
            - dev.DOMAIN_NAME
        WebsiteConfiguration:
          IndexDocument: index.html
          ErrorDocument: error.html
@amberlion

This comment has been minimized.

amberlion commented Sep 14, 2017

When I used this branch, I found a problem.

serverless.yml

service: VpcDefault

provider:
  name: aws

custom:
  VpcDefault: ${file(./child.yml):custom.VpcDefault}
resources:
  Resources:
    Vpc:
      Type: "AWS::EC2::VPC"
      Properties:
        CidrBlock: ${self:custom.VpcDefault.resource.Properties.CidrBlock}
        EnableDnsSupport: true
        EnableDnsHostnames: true
        InstanceTenancy: "default"
        Tags: ${self:test, self:custom.VpcDefault.resource.Properties.Tags}


child.yml

custom:
  VpcDefault:
    resource:
      Properties:
        CidrBlock: "192.168.0.0/16"
        Tags:
          - Key: Name
            Value: ${self:provider.stage}-Vpc

I executed sls package between 2versions.(v1.22.0/circular-vars)
and circular-vars couldn't parse ${self:provider.stage}.

{
  "AWSTemplateFormatVersion": "2010-09-09",
  "Description": "The AWS CloudFormation template for this Serverless application",
  "Resources": {
    "ServerlessDeploymentBucket": {
      "Type": "AWS::S3::Bucket"
    },
    "Vpc": {
      "Type": "AWS::EC2::VPC",
      "Properties": {
        "CidrBlock": "192.168.0.0/16",
        "EnableDnsSupport": true,
        "EnableDnsHostnames": true,
        "InstanceTenancy": "default",
        "Tags": [
          {
            "Key": "Name",
            "Value": "${self:provider.stage}-Vpc"
          }
        ]
      }
    }
  },
  "Outputs": {
    "ServerlessDeploymentBucketName": {
      "Value": {
        "Ref": "ServerlessDeploymentBucket"
      }
    }
  }
}
diff cloudformation-template-update-stack-circular-vars.json cloudformation-template-update-stack-v1.22.0.json
18c18
<             "Value": "${self:provider.stage}-Vpc"
---
>             "Value": "dev-Vpc"

If you change ${self:test, self:custom.VpcDefault.resource.Properties.Tags} to
${self:custom.VpcDefault.resource.Properties.Tags},
this problem doesn't occur.

If you don't use child.yml, this problem doesn't occur too.

@eahefnawy eahefnawy removed this from the 1.23 milestone Sep 21, 2017

@erikerikson

This comment has been minimized.

Member

erikerikson commented Nov 8, 2017

conflicts resolved in #4412

FWIW, this bug has been unresolved since '1.12.0': '2017-04-26T16:01:23.822Z'

@erikerikson erikerikson referenced this pull request Dec 5, 2017

Merged

Add significant variable usage corner cases #4529

4 of 7 tasks complete

HyperBrain and others added some commits Dec 6, 2017

Merge pull request #4529 from erikerikson/circular-vars-test
Add significant variable usage corner cases
Refactor, simplify, correct, add tests
Increase name consistency
Improve names to be-what-they-are
Reduce cyclomatic complexity
Include overrides population fix
Add variable methods documentation
Add more tests around significant variable patterns/risky cases
@HyperBrain

This comment has been minimized.

Member

HyperBrain commented Dec 7, 2017

Ok, now we have everything new integrated (changes done in master) and highly covered unit test use-cases. It's time to do some final tests now.
/cc @pmuens @horike37 @RafalWilinski

erikerikson and others added some commits Dec 8, 2017

add tests to get to 100% line coverage
1. add tests that validate string value defaults in overrides.
2. add test of behavior when a javascript's export does not contain the attribute declared in the variable.
3. add tests of behavior when an SSM parameter is not found and the request to the SSM service fails.

add returns to tests that weren't returning their promises (and were thereby risking false positives.
Merge pull request #4541 from erikerikson/add-coverage
add tests to get to 100% line coverage
@horike37

I tested it locally and the package and deploy commands worked fine perfectly.

However, the 'print' command has failed if ${self:} is included within serverless.yml. it would not be a critical bug since the command doesn't directly affect deployments.

However, it would be good to fix it so that doesn't let users confusing.

Here is the yml file to reproduce it and the error I saw.

serverless.yml

service:
  name: s1

custom:
  varQ: ${self:} # referencing the entire config file, NEW!

provider:
  name: aws
  runtime: nodejs6.10

functions:
  f1:
    handler: handler.hello

error

 Range Error --------------------------------------------
 
  Maximum call stack size exceeded
 
     For debugging logs, run again after setting the "SLS_DEBUG=*" environment variable.
 
  Stack Trace --------------------------------------------
 
RangeError: Maximum call stack size exceeded
    at RegExp.exec (<anonymous>)
    at Type.resolveYamlTimestamp [as resolve] (/Users/horike/src/serverless/node_modules/js-yaml/lib/js-yaml/type/timestamp.js:25:29)
    at testImplicitResolving (/Users/horike/src/serverless/node_modules/js-yaml/lib/js-yaml/dumper.js:166:14)
    at testAmbiguity (/Users/horike/src/serverless/node_modules/js-yaml/lib/js-yaml/dumper.js:344:14)
    at chooseScalarStyle (/Users/horike/src/serverless/node_modules/js-yaml/lib/js-yaml/dumper.js:300:22)
    at /Users/horike/src/serverless/node_modules/js-yaml/lib/js-yaml/dumper.js:347:13
    at writeScalar (/Users/horike/src/serverless/node_modules/js-yaml/lib/js-yaml/dumper.js:363:4)
    at writeNode (/Users/horike/src/serverless/node_modules/js-yaml/lib/js-yaml/dumper.js:744:9)
    at writeBlockMapping (/Users/horike/src/serverless/node_modules/js-yaml/lib/js-yaml/dumper.js:606:10)
    at writeNode (/Users/horike/src/serverless/node_modules/js-yaml/lib/js-yaml/dumper.js:720:9)
    at writeBlockMapping (/Users/horike/src/serverless/node_modules/js-yaml/lib/js-yaml/dumper.js:627:10)
    at writeNode (/Users/horike/src/serverless/node_modules/js-yaml/lib/js-yaml/dumper.js:720:9)
    at writeBlockMapping (/Users/horike/src/serverless/node_modules/js-yaml/lib/js-yaml/dumper.js:627:10)
    at writeNode (/Users/horike/src/serverless/node_modules/js-yaml/lib/js-yaml/dumper.js:720:9)
    at writeBlockMapping (/Users/horike/src/serverless/node_modules/js-yaml/lib/js-yaml/dumper.js:627:10)
    at writeNode (/Users/horike/src/serverless/node_modules/js-yaml/lib/js-yaml/dumper.js:720:9)
    at writeBlockMapping (/Users/horike/src/serverless/node_modules/js-yaml/lib/js-yaml/dumper.js:627:10)
    at writeNode (/Users/horike/src/serverless/node_modules/js-yaml/lib/js-yaml/dumper.js:720:9)
    at writeBlockMapping (/Users/horike/src/serverless/node_modules/js-yaml/lib/js-yaml/dumper.js:627:10)
    at writeNode (/Users/horike/src/serverless/node_modules/js-yaml/lib/js-yaml/dumper.js:720:9)
    at writeBlockMapping (/Users/horike/src/serverless/node_modules/js-yaml/lib/js-yaml/dumper.js:627:10)
    at writeNode (/Users/horike/src/serverless/node_modules/js-yaml/lib/js-yaml/dumper.js:720:9)
    at writeBlockMapping (/Users/horike/src/serverless/node_modules/js-yaml/lib/js-yaml/dumper.js:627:10)
    at writeNode (/Users/horike/src/serverless/node_modules/js-yaml/lib/js-yaml/dumper.js:720:9)

@HyperBrain

This comment has been minimized.

Member

HyperBrain commented Dec 9, 2017

@horike37 Many thanks for the testing 💯 . 6 eyes definitely see more than 4 😄 .
@erikerikson It seems that the way, the print command is implemented leads to an infinite recursion in this case (RangeError: Maximum call stack size exceeded). Imo we should try to find the issue there (within print) so that the print command itself can handle self references too. I'm quite sure that it just traverses the objects but does not recognize if it already visited a node. Maybe it should use the circular aware JSON module (I think it was jc-..,).

@erikerikson

This comment has been minimized.

Member

erikerikson commented Dec 9, 2017

Yes, that seems exactly right @HyperBrain. Which probably means this is an existing bug that we've missed until now. It's a good and simple one to fix. I can take a look but am not sure what kind of guarantee I can give about timing during this upcoming week although I really want to get these changes in. I suspect modifying what is fed into js-yaml/dumper is the way to go although I suppose it wouldn't be bad to contribute that support upstream. It'll be in the package.json but I think it's json-cycle which we usually assign to a variable jc.

@HyperBrain

This comment has been minimized.

Member

HyperBrain commented Dec 10, 2017

@horike37 @erikerikson @pmuens If everyone agrees, I want to propose the following:
We merge this PR in as it is (the variable system is in a much better shape than before and much more stable) and decouple the self reference issue with the print command (create a separate issue for that).
I consider that as an edge case that from a pragmatic perspective can be delivered in a subsequent update of the next release.

This allows us to get out a 1.25 next week, that includes lots of long wanted fixes and stabilization improvements. If everyone agrees, please approve your reviews and I'll create a separate issue for print afterwards - and of course merge the PR, finally.

Additionally I'll fix the upcoming conflicts in #4537 and merge that in too, to have the lean request signature back. Then we're completely ready for 1.25.

@HyperBrain

Approved under the assumption of my last comment

@horike37

This comment has been minimized.

Member

horike37 commented Dec 10, 2017

Thanks @HyperBrain for taking the lead on this 👍
Fully agreed with your last comment 😄

@HyperBrain

This comment has been minimized.

Member

HyperBrain commented Dec 11, 2017

I created a separate bug for the print issue.

Has been addressed

@HyperBrain HyperBrain merged commit be68dee into master Dec 11, 2017

5 checks passed

continuous-integration/appveyor/branch AppVeyor build succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.2%) to 88.122%
Details
@HyperBrain

This comment has been minimized.

Member

HyperBrain commented Dec 11, 2017

Merged :)

@pmuens pmuens deleted the circular-vars branch Dec 20, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment