Plugins - now allow falsy default values.

[neverendingqs]

What did you implement

Previously, the plugin manager did not properly set the default value if the default value is 0, false, or ''. This PR fixes that.

Closes #XXXXX (none that I can find)

How can we verify it

serverless.yml:

service: foo

provider:
  name: aws
  runtime: nodejs10.x

plugins:
  - serverless-test-plugin

.serverless_plugins/serverless-test-plugin.js:

'use strict';

class TestPlugin {
  constructor(serverless, options) {
    this.options = options;
    this.commands = {
      options: {
        usage: 'Options viewer',
        lifecycleEvents: ['options'],
        commands: {
          log: {
            usage: 'Log all options',
            options: {
              zero: { default: 0 },
              'empty-string': { default: '' },
              false: { default: false }
            },
            lifecycleEvents: ['log']
          }
        }
      }
    };

    this.hooks = {
      'options:log:log': () => this.log()
    };
  }

  log() {
    console.log('log()');
    console.log(JSON.stringify(this.options));
  }
}

module.exports = TestPlugin;

Then run

sls options log

Before:

log()
{}

After:

log()
{"zero":0,"empty-string":"","false":false}

Todos

Useful Scripts

• npm run test:ci --> Run all validation checks on proposed changes
• npm run lint:updated --> Lint all the updated files
• npm run lint:fix --> Automatically fix lint problems (if possible)
• npm run prettier-check:updated --> Check if updated files adhere to Prettier config
• npm run prettify:updated --> Prettify all the updated files

• Write and run all tests
• Write documentation
• Enable "Allow edits from maintainers" for this PR
• Update the messages below

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

[codecov-io]

Codecov Report

Merging #7071 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #7071      +/-   ##
==========================================
+ Coverage   88.48%   88.48%   +<.01%     
==========================================
  Files         231      231              
  Lines        8478     8479       +1     
==========================================
+ Hits         7502     7503       +1     
  Misses        976      976

Impacted Files	Coverage Δ
lib/classes/PluginManager.js	96.67% <100%> (ø)	⬆️
lib/plugins/aws/package/compile/functions/index.js	95.45% <0%> (-0.38%)	⬇️
...plugins/aws/package/compile/events/stream/index.js	98.79% <0%> (-0.02%)	⬇️
lib/plugins/aws/lib/naming.js	99.21% <0%> (ø)	⬆️
lib/plugins/aws/lib/validate.js	100% <0%> (+4%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update be8e64c...d8f4b5b. Read the comment docs.

[pmuens]

Hey @neverendingqs thanks for working on this 👍

The code looks good so far. The only thing I'm concerned about is whether this change would break existing plugins which rely on the current behavior of falsy default values.

@medikoo what are your thoughts around that? 🤔

[medikoo]

@neverendingqs great thanks for that PR. It's a very good call!.

I've suggested one improvement, so we handle it more naturally (where JS language is concerned)

[medikoo]

The only thing I'm concerned about is whether this change would break existing plugins which rely on the current behavior of falsy default values.

Yes, technically if one put default: false as a way to not provide a default value, it'll break.
However I'd say it's quite a corner case, and language wise, fact that we didn't support falsy values as default looks like a bug (I'd see default as type of any and not a boolean, and for any, it's only null and undefined that should be treated as no input).

I would drive it in, as bug fix

[pmuens]

Yes, technically if one put default: false as a way to not provide a default value, it'll break.
However I'd say it's quite a corner case, and language wise, fact that we didn't support falsy values as default looks like a bug (I'd see default as type of any and not a boolean, and for any, it's only null and undefined that should be treated as no input).

100% agree with that sentiment 👍 I'd also treat it as a bug rather than a feature.

[medikoo]

Thank you @neverendingqs, looks great!