Add logDeprecation and tests by AhmedFat7y · Pull Request #7741 · serverless/serverless

AhmedFat7y · 2020-05-17T19:32:19Z

Closes: #7422

I'm not sure if this is what you're expecting, but wanted to push this so we can discuss the code better than just a theory

medikoo

Thank you @AhmedFat7y looks promising! See my suggestions

medikoo · 2020-05-18T09:21:41Z

+
+function logDeprecation(code, message, docURL, serviceConfig) {
+  const disabledCodes = prepareCodes(
+    serviceConfig && serviceConfig.disabledCodes,


I would create one dedicated function to resolve deprecated codes from serviceConfig. I wouldn't mix it with resolution from process.env as it's different, and settings from process.env can be resolved upfront in module body

The idea was to keep the codes preparation separate and explicit so you know what was prepared for this call explicitly. The env codes and serviceCodes (if they exist) were extracted in single call to make it easier to trace what was extracted
I assume serviceConfig.disabledCodes will always return either an empty Set or a Set with codes to disable. So I can use it without taking too much care about it being sent or not.

I separated them, but why do you want to keep the serviceConfig codes resolution and checking that separate?

The env codes and serviceCodes (if they exist) were extracted in single call to make it easier to trace what was extracted

I understand the intention, still both values are provided at different points in time, and format of both differs. So resolving both with one function, doesn't seem clean to me.

I believe that process.env can be resolved upfront in module body, so we have upfront (for function processing) ready set of codes deprecated by env settings. I think it'll make further processing of that cleaner.

process.env format is in all cases a string, which eventually may contain multiple coma separated values. While in case of serviceConfig, it can be either string (singular value) or array. So resolution of both should in any case be done with different logic (different function)

While in case of serviceConfig, it can be either string (singular value) or array

So it will be provided like this?

provider: aws disabledCodes: - code1 - code2

I thought it will be comma separated values like the env var

provider: aws disabledCodes: code1,code2,code3

Or it will be processed by service initialisation? Because I assumed it should match how it's provided to the Env var

In #7422 I've proposed to name it disabledDeprecations, and concerning format we should respect convention in how single and multiple valuas are configured in YAML (which translates to JSON).

Still I think it's fine to support just one format (an array), at least singular string will look confusing by disabledDeprecations which suggests plural.

So let's support just:

disabledDeprecations: - CODE1 - CODE2

And for disabledDeprecations let's make a one test that is configured viarunServerless (only that way we should test input coming via serviceConfig)

In #7422 I've proposed to name it disabledDeprecations, and concerning format we should respect convention in how single and multiple valuas are configured in YAML (which translates to JSON).

Sorry for that, I was trying to get sth submitted to discuss before finalizing the implementation

Still I think it's fine to support just one format (an array), at least singular string will look confusing by disabledDeprecations which suggests plural.

You always get the request to support '*' as a single value instead of a single array item. It doesn't matter here?

You always get the request to support '*' as a single value instead of a single array item. It doesn't matter here?

Good point! I forgot about that. In that case let's support also single value notation where a string is assigned to disabledDeprecations directly.

Also I wonder, maybe more correct name would be disabledDeprecationNotifications (at least disabledDeprecations suggest we disable deprecations which is not perfectly correct). What do you think?

I added the support for single value either way until you reply

codecov-io · 2020-05-18T13:50:59Z

Codecov Report

❗ No coverage uploaded for pull request base (master@304a502). Click here to learn what that means.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##             master    #7741   +/-   ##
=========================================
  Coverage          ?   88.02%           
=========================================
  Files             ?      245           
  Lines             ?     9234           
  Branches          ?        0           
=========================================
  Hits              ?     8128           
  Misses            ?     1106           
  Partials          ?        0

Impacted Files	Coverage Δ
lib/classes/Service.js	`97.52% <100.00%> (ø)`
lib/utils/logDeprecation.js	`100.00% <100.00%> (ø)`
lib/utils/fs/removeFileSync.js	`100.00% <0.00%> (ø)`
lib/utils/getTrackingConfigFileName.js	`0.00% <0.00%> (ø)`
...s/create/templates/aws-alexa-typescript/handler.ts	`0.00% <0.00%> (ø)`
lib/plugins/remove/remove.js	`77.77% <0.00%> (ø)`
lib/components-v2.js	`40.00% <0.00%> (ø)`
lib/plugins/aws/customResources/index.js	`98.59% <0.00%> (ø)`
commitlint.config.js	`0.00% <0.00%> (ø)`
lib/plugins/create/templates/plugin/index.js	`0.00% <0.00%> (ø)`
... and 237 more

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 304a502...23a50a5. Read the comment docs.

AhmedFat7y · 2020-05-18T14:06:01Z

@medikoo I added support for disbledDeprecation in https://github.com/AhmedFat7y/serverless/blob/23a50a5d16e9613c9779eb5fd24ce71dca77245b/lib/classes/Service.js#L113-L118

Let me know what your final input on supporting singular value
Should I validate strictly that deprecations is an array. In other words, should it crash if object was provided for some reason?
I would add test if I validated strictly, but should I add tests to test support for array and singular value?

medikoo · 2020-05-18T14:10:23Z

Should I validate strictly that deprecations is an array. In other words, should it crash if object was provided for some reason?

No, any validation will be in hands of schema based config validation (handled in #6562).

Here I imagine setup as:

const schemaDeprecations = new Set([
  typeof serviceConfig.disabledDeprecations === 'string'
    ? serviceConfig.disabledDeprecations
    : ...(serviceConfig.disabledDeprecations || [])
]);

AhmedFat7y · 2020-05-18T14:20:23Z

Should I validate strictly that deprecations is an array. In other words, should it crash if object was provided for some reason?

No, any validation will be in hands of schema based config validation (handled in #6562).

Here I imagine setup as:
const schemaDeprecations = new Set([
  typeof serviceConfig.disabledDeprecations === 'string'
    ? serviceConfig.disabledDeprecations
    : ...(serviceConfig.disabledDeprecations || [])
]);

So it's better to handle that support in the logDeprecation function as it's what will be parsing it anyways. Gotchaa

medikoo

@AhmedFat7y looks great! I'm pretty excited to have that in soon, as it'll open a new great way towards new major releases.

Please see my final and rather minor remarks.

medikoo · 2020-05-20T11:36:35Z

 frameworkVersion: '>=1.0.0 <2.0.0'

+disabledDeprecations: # Disable deprecation logs by their codes. Default is empty.
+  - code1 # Deprecation code to disable


Let's use DEP_CODE_1 instead of code1, it'll be more similar to deprecation code format

medikoo · 2020-05-20T11:40:08Z

+  if (typeof serviceConfig.disabledDeprecations === 'string') {
+    disabledDeprecations = [serviceConfig.disabledDeprecations];
+  } else {
+    disabledDeprecations = serviceConfig.disabledDeprecations || [];


To be on safer side, let's do Array.from(serviceConfig.disabledDeprecations || [])

Otherwise if e.g. a plain object is assigned to disabledDeprecations, it'll crash when it's passed to new Set, and we don't want crashes here

I was planning to ask about that in a comment, but I think I didn't press the comment button.

Should we display a warning in case an object is passed for some reason? Or we ignore it for now until the schema validation is done?

Should we display a warning in case an object is passed for some reason? Or we ignore it for now until the schema validation is done?

Ignore it. It's also per JS convention to accept objects as empty arrays when array was expected.

medikoo · 2020-05-20T11:41:44Z

+  return new Set(codesStr.split(','));
+}
+
+const resolveDeprecatedByService = memoizee((serviceConfig = {}) => {


In this case serviceConfig should be treated as required argument (not default).

Note that configured as above, memoization will ignore serviceConfig argument, and will memoize function as if takes no arguments (only first invocation will be a cache miss, all others no matter what service config is passed will result with cache hit).

When I try this simple demo

const memoize = require('memoizee'); const func = memoize(function (obj) { return Object.keys(obj); }); console.log(func({e: 1})) console.log(func({y: 1}))

This will print [ 'e' ] & [ 'y' ] respectively

But this demo doesn't reflect what you've configured. Try below instead :)

const memoize = require('memoizee'); const func = memoize(function (obj = {}) { return Object.keys(obj); }); console.log(func({e: 1})) console.log(func({y: 1}))

medikoo · 2020-05-20T11:42:27Z

@@ -0,0 +1,51 @@
+'use strict';
+const chalk = require('chalk');
+const memoizee = require('memoizee');


It'll be cleaner to use memoizee/weak, then memoization will no lock service objects in a memory

medikoo · 2020-05-20T11:43:15Z

+        `More Info: https://www.serverless.com/framework/docs/deprecations/#${code}`
+      )}}`,
+    ].join('\n')
+  );


Let's also add new line at the end (in case of process.stdout.write they're not added as it's with console.log

medikoo · 2020-05-20T11:44:05Z

+  const serviceDisabledCodes = resolveDeprecatedByService(serviceConfig);
+  if (serviceDisabledCodes.has(code) || serviceDisabledCodes.has('*')) {
+    return;
+  }


Let's wrap it into if (serviceConfig) conditional

codecov-commenter · 2020-05-20T19:17:32Z

⚠️ Please install the to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.03%. Comparing base (3e446f0) to head (dc52a44).

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #7741      +/-   ##
==========================================
+ Coverage   88.00%   88.03%   +0.03%     
==========================================
  Files         245      246       +1     
  Lines        9254     9279      +25     
==========================================
+ Hits         8144     8169      +25     
  Misses       1110     1110

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Resolve env codes in module body Re-require inside test body for env override Use stdout.write Change format to match provided in comments

Use Array.from to avoid errors in case the codes were an object Write new line as stdout.write doesn't print one by default Wrap serviceConfig resolution in if condition

Add new test to check silent failure in case of codes are an object

medikoo · 2020-05-21T11:24:35Z

@AhmedFat7y Thank you, looks really good! Before I merge it, I want to be sure we have website page towards deprecations configured. I'll get back to it shortly once that's setup

medikoo

Thank you @AhmedFat7y !

I've added some minor tweaks, and fixed documentation page so it builds properly with our Gatsby setup.

I'll follow with simple PR that configures first depracation, it'll give an example on how deprecations should be configured

medikoo suggested changes May 18, 2020

View reviewed changes

AhmedFat7y force-pushed the add-log-deprecation branch 4 times, most recently from 5b3312a to 23a50a5 Compare May 18, 2020 13:48

AhmedFat7y force-pushed the add-log-deprecation branch 2 times, most recently from d5dcb3b to 330c1d1 Compare May 18, 2020 14:44

AhmedFat7y requested a review from medikoo May 20, 2020 09:23

medikoo suggested changes May 20, 2020

View reviewed changes

AhmedFat7y force-pushed the add-log-deprecation branch from 7227afc to 601fa5c Compare May 20, 2020 19:08

AhmedFat7y and others added 8 commits May 21, 2020 00:06

Add logDeprecation and tests

ce448a0

Separate env codes resolution from serviceConfig.

5b9f488

Resolve env codes in module body Re-require inside test body for env override Use stdout.write Change format to match provided in comments

Add support for disabledDeprecations in Service

fbf2a78

Use runServerless to test serviceConfig deprecations

b57db8a

Add docs

52aa3a2

Use memoizee/weak to bind cache to objects

4138f35

Use Array.from to avoid errors in case the codes were an object Write new line as stdout.write doesn't print one by default Wrap serviceConfig resolution in if condition

Fix test based on stdoutWriteSpy call count to match added newline

4b6d1eb

Add new test to check silent failure in case of codes are an object

Make code sample more similar to how it will be

22af615

AhmedFat7y force-pushed the add-log-deprecation branch from 601fa5c to 22af615 Compare May 20, 2020 22:06

AhmedFat7y requested a review from medikoo May 20, 2020 22:20

medikoo added 4 commits May 21, 2020 15:12

style: whitespace

264debb

refactor: Simplify export

ac409fc

fix: Remove invalid character

419761f

feat: Improve message presence

b6deb61

medikoo added 3 commits May 21, 2020 15:18

feat: Improve deprecation message

32d41fe

docs: Fix deprecation docs setup

25dea97

test: Fix after recent updates

dc52a44

medikoo approved these changes May 21, 2020

View reviewed changes

medikoo added cat/cli enhancement labels May 21, 2020

medikoo merged commit 6f32f23 into serverless:master May 21, 2020

AhmedFat7y deleted the add-log-deprecation branch May 21, 2020 15:51

Uh oh!

Conversation

AhmedFat7y commented May 17, 2020

Uh oh!

medikoo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

medikoo May 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

medikoo May 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

codecov-io commented May 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

AhmedFat7y commented May 18, 2020

Uh oh!

medikoo commented May 18, 2020

Uh oh!

AhmedFat7y commented May 18, 2020

Uh oh!

medikoo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

medikoo May 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov-commenter commented May 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

medikoo May 18, 2020 •

edited

Loading

medikoo May 18, 2020 •

edited

Loading

codecov-io commented May 18, 2020 •

edited

Loading

medikoo May 20, 2020 •

edited

Loading

codecov-commenter commented May 20, 2020 •

edited

Loading

medikoo commented May 21, 2020 •

edited

Loading