Support non-function exports when including variables from js files #7540

srapp · 2020-04-03T20:57:42Z

What did you implement

Currently, when you import a .js file in your configuration, serverless only supports functions. This change also allows referencing non-function exports from a .js file.

Closes #7443

How can we verify it

config.js

module.exports = {
  foo: 'bar',
  doesWork: true
}

serverless.yml

service: test
provider:
  name: 'test'
custom:
  from-js: ${file(./config.js)}

Command:

sls print

Expected output:

service: test
provider:
  name: test
custom:
  from-js:
    foo: bar
    doesWork: true

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

srapp · 2020-04-03T21:00:42Z

I held off on adding any documentation until I get some feedback on whether this approach is in the right direction. Happy to contribute some once it's been given a look.

medikoo · 2020-04-21T13:22:14Z

@srapp thanks for this PR. In general it looks fine to me, although still use case is not perfectly clear.

@erikerikson what do you think about that proposal? Do you see any implications it may introduce?

erikerikson

I haven't looked deeply and don't recall the history of the choice to only allow a function to be returned. I think it would probably be helpful to look up the changes to the code over time and the associated discussions.

I suspect that, as hinted by my comments on the code, the implementer was looking at the diversity of possible returns and chose to keep it simple. Perhaps they even started by accepting the return of objects but realized they could get concrete values but also things that couldn't easily be dealt with or identified. Finally, I suspect that as they started to build out the code to deal with that, they simplified to avoid an unexpected expansion of scope and work. Maybe they just wanted to return a function.

Personally, it seems like a fine thing to increase the richness of what the JavaScript can return. I believe that historically there has been a constraint that the module code must be synchronous. Maybe my thoughts about handling specific return type constraints is pedantic. If so, apologies, I'm just waking up, on my phone, and avoiding going deep.

lib/classes/Variables.test.js

erikerikson · 2020-04-21T14:14:16Z

lib/classes/Variables.js

      } else {
-        returnValueFunction = jsFile;
+        returnValue = jsFile;


I covered this in my comment on 1914 but you may not have an object or even a concrete value. What if jsFile === Error('')?

Totally possible, technically an Error would be an object, though perhaps not one that we'd want to insert the values of. Same issue could happen though if a function returned an Error too. So what should be the handling of that?

I think we could probably continue building a list of things we don't want to process that are technically objects, and I think we can't prevent the users from returning whatever they want. I'm not sure whether we want to try to keep up with a list of things we don't want to explicitly support, or rather assume the user is only going to be returning useful values for configuration. If an error has occurred - I think the advice would be to throw that error, rather than return it.

As for when an error is thrown, I think currently we just let that bubble up normally, though it's not wrapped in a Serverless error class at the moment. We could do some try/catching here to wrap it if that makes sense.

I think you're calling me out for being a bit too pedantic and I agree. Reasonableness and dev awareness was assumed before and I shouldn't ask for elevation above that previous bar. The previous paradigm was to check for the one type of valid return (i.e. a function) but not to vet the result of the function's execution. Perhaps some positive case checking makes sense but since a non-function return is the equivalent of the function return value, I feel like I have little standing to request more. Mariusz may have a stronger opinion.

For the record and what it's worth, I don't have merge rights and can't give the green light so I'm just attempting to be helpful. Despite that, sorry for the slow responses.

@erikerikson great thanks for that input. Indeed there can be anything exported by a module (even undefined).

But as long as we do not validate the value that is returned by eventual function resolver, for consistency we also should not validate a directly provided value.

I think real concern comes from a design proposal: where we treat a given value as resolved as long as it's not a function and otherwise we treat a function as value resolver.

I have mixed feelings on that, it definitely can turn to be confusing, e.g. in current form we accept also a function as final resolved value (it can be returned by function resolver)

It'll work better if we restrict and validate the final valueToPopulateResolved. Ideally we should just support JSON values, and simplified validation of that would be to reject all values for which typeof returns 'undefined, 'symbol' or 'function'.

@erikerikson, @medikoo thank you both for the feedback! Sorry it's taken a bit to respond, I'll try to stay on top of this PR a bit better 😅

@medikoo I think we're in agreement here - that currently we don't really check the type of valueToPopulateResolved, regardless of how that value is provided (function invocation or otherwise). Focusing on valueToPopulateResolved and evaluating whether we've landed on a valid result type makes a lot of sense to me.

If I'm not mistaken, it looks like we already handle this for undefined values: https://github.com/serverless/serverless/blob/master/lib/classes/Variables.js#L728. I've updated this PR to also throw an error if the resolved value is a symbol or function.

lib/classes/Variables.test.js

medikoo · 2020-05-19T08:51:03Z

@srapp is this ready to take? If so please re-request review. Thank you!

srapp · 2020-05-19T15:10:37Z

@medikoo Yes it is, barring documentation. I'll see if I can draft the docs this afternoon

srapp · 2020-05-19T19:30:54Z

@medikoo I've made a minor change to the docs, just modifying the current example to export a constant value in one case, rather than a function.

codecov-commenter · 2020-05-19T19:39:48Z

Codecov Report

Merging #7540 into master will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #7540      +/-   ##
==========================================
+ Coverage   87.99%   88.00%   +0.01%     
==========================================
  Files         241      245       +4     
  Lines        9120     9253     +133     
==========================================
+ Hits         8025     8143     +118     
- Misses       1095     1110      +15

Impacted Files	Coverage Δ
lib/classes/Variables.js	`99.72% <100.00%> (+<0.01%)`	⬆️
lib/utils/getTrackingConfigFileName.js	`0.00% <0.00%> (-100.00%)`	⬇️
lib/plugins/aws/info/getResourceCount.js	`90.00% <0.00%> (-10.00%)`	⬇️
.../aws/package/compile/events/alb/lib/permissions.js	`92.85% <0.00%> (-7.15%)`	⬇️
lib/utils/npm-command-deferred.js	`75.00% <0.00%> (-2.78%)`	⬇️
lib/plugins/aws/package/compile/layers/index.js	`95.60% <0.00%> (-2.61%)`	⬇️
.../compile/events/apiGateway/lib/hack/updateStage.js	`94.96% <0.00%> (-1.66%)`	⬇️
lib/plugins/aws/deploy/lib/uploadArtifacts.js	`89.23% <0.00%> (-1.25%)`	⬇️
lib/plugins/executable/index.js	`33.33% <0.00%> (-0.76%)`	⬇️
...ins/aws/package/compile/events/cloudFront/index.js	`99.31% <0.00%> (-0.69%)`	⬇️
... and 36 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 26cc120...dae73d6. Read the comment docs.

medikoo

@srapp looks great! I've proposed just one minor style improvement, and we should be good to go

medikoo · 2020-05-20T09:07:32Z

lib/classes/Variables.js


      return BbPromise.resolve(valueToPopulate).then(valueToPopulateResolved => {
        let deepProperties = variableString.replace(matchedFileRefString, '');
        deepProperties = deepProperties.slice(1).split('.');
        deepProperties.splice(0, 1);
        return this.getDeeperValue(deepProperties, valueToPopulateResolved).then(
          deepValueToPopulateResolved => {
-            if (typeof deepValueToPopulateResolved === 'undefined') {
+            if (['undefined', 'symbol', 'function'].includes(typeof deepValueToPopulateResolved)) {


Let's seclude that to maybe nonJsonTypes set in module body, and validate via:

if (nonJsonTypes.has(typeof deepValueToPopulateResolved)) {

medikoo

Thank you @srapp !

Support non-function exports when including variables from js files

0c665cc

erikerikson reviewed Apr 21, 2020

View reviewed changes

medikoo reviewed Apr 27, 2020

View reviewed changes

lib/classes/Variables.test.js Outdated Show resolved Hide resolved

Steven Rapp added 2 commits May 7, 2020 16:33

Add more test cases, protection against symbols and functions

1dddc8d

Fix prettier exceptions

d05c171

srapp requested a review from medikoo May 19, 2020 15:10

Update docs

dae73d6

srapp force-pushed the js-export-non-functions branch from efc99a3 to dae73d6 Compare May 19, 2020 19:30

medikoo suggested changes May 20, 2020

View reviewed changes

medikoo added cat/variable enhancement labels May 20, 2020

medikoo assigned srapp May 20, 2020

Introduced nonJsonTypes set into module

b9c9cbe

srapp requested a review from medikoo May 20, 2020 21:12

medikoo approved these changes May 21, 2020

View reviewed changes

medikoo merged commit 89ba272 into serverless:master May 21, 2020

srapp deleted the js-export-non-functions branch May 21, 2020 17:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support non-function exports when including variables from js files #7540

Support non-function exports when including variables from js files #7540

srapp commented Apr 3, 2020 •

edited

srapp commented Apr 3, 2020

medikoo commented Apr 21, 2020

erikerikson left a comment

erikerikson Apr 21, 2020

srapp Apr 21, 2020 •

edited

erikerikson Apr 27, 2020

medikoo Apr 27, 2020 •

edited

srapp May 7, 2020

medikoo commented May 19, 2020

srapp commented May 19, 2020

srapp commented May 19, 2020

codecov-commenter commented May 19, 2020

medikoo left a comment

medikoo May 20, 2020

medikoo left a comment

Support non-function exports when including variables from js files #7540

Support non-function exports when including variables from js files #7540

Conversation

srapp commented Apr 3, 2020 • edited

What did you implement

How can we verify it

Todos

srapp commented Apr 3, 2020

medikoo commented Apr 21, 2020

erikerikson left a comment

Choose a reason for hiding this comment

erikerikson Apr 21, 2020

Choose a reason for hiding this comment

srapp Apr 21, 2020 • edited

Choose a reason for hiding this comment

erikerikson Apr 27, 2020

Choose a reason for hiding this comment

medikoo Apr 27, 2020 • edited

Choose a reason for hiding this comment

srapp May 7, 2020

Choose a reason for hiding this comment

medikoo commented May 19, 2020

srapp commented May 19, 2020

srapp commented May 19, 2020

codecov-commenter commented May 19, 2020

Codecov Report

medikoo left a comment

Choose a reason for hiding this comment

medikoo May 20, 2020

Choose a reason for hiding this comment

medikoo left a comment

Choose a reason for hiding this comment

srapp commented Apr 3, 2020 •

edited

srapp Apr 21, 2020 •

edited

medikoo Apr 27, 2020 •

edited