1.18.0 breaks referencing variables in Javascript Files

[indieisaconcept]

This is a Bug Report

Description

This might be considered an edge case based upon how I make use of a JavaScript configuration file but thought I would share this issue nonetheless as it tripped me up when I went to do a deploy this evening after updating my deps.

Consider the following contrived example.

config.js

const env    = process.env.NODE_ENV || 'dev';
const config = { destinationStream : `kinesis-stream-${env}` }

config._get = function () { return this; }
module.exports = config;

serverless.yml

custom :
    destinationStream : ${file(./config.js):_get.destinationStream}

cmd to reproduce

> NODE_ENV=staging serverless package

Running the above cmd results in the following error.

Invalid variable syntax when referencing file "./config.js". Check if your javascript is returning the correct data.

Note this issue did not exist prior to 1.18.

I've reviewed the PR's which made up 1.18 and have pin-pointed #3888 as being the change which introduced this new behaviour.

Prior to 1.18 when a JS file was resolved it was immediately invoked and so it's context was maintained. However in #3888 it is assigned to a local variable and so no-longer has it's original execution context. ( see lib/classes/Variables#L255 )

This can be fixed by amending L255 to :

valueToPopulate = returnValueFunction.call(jsFile);

I'd be happy to submit a PR for this if you think it's worth fixing. I've since worked around it but as per above I thought I'd share the info.

Additional Data

• Serverless Framework Version you're using: 1.18
• Operating System: 10.12.5
• Stack Trace: N/A
• Provider Error messages: N/A

[eahefnawy]

@indieisaconcept thanks for reporting! thats a pretty creative edge case! have you been able to reproduce it with a simpler config? just wondering if it's a bigger issue than we think.

a PR with the fix would be great! 😊

[jeanbza]

We're seeing this too. Libraries like request rely on it somehow, and are breaking. Reverting to 1.17 fixes the issue.

[indieisaconcept]

@eahefnawy I don't expect it to be an issue if your not doing any additional processing from within a *.js variables file - so a basic config should be fine. I'll confirm though.

As this is a simple fix, I'll submit a PR shortly.

[pmuens]

Thanks for getting back @indieisaconcept and thanks for working on and submitting the PR 👍 🎉

We'll review it ASAP so that the fix lands in master soon!

[maikokuppe]

The version 1.18.0 broke my deployment too, though I did not dig for the issue. As a reference for help-seeking googling developers, I'm posting this unhelpful error message from my lambda logs, which was caused by the version update:

Unable to import module 'handler': Error
    at Function.Module._resolveFilename (module.js:469:15)
    at Function.Module._load (module.js:417:25)
    at Module.require (module.js:497:17)
    at require (internal/module.js:20:19)
    at Object.<anonymous> (/var/task/node_modules/github/lib/index.js:5:23)
    at Module._compile (module.js:570:32)
    at Object.Module._extensions..js (module.js:579:10)
    at Module.load (module.js:487:32)
    at tryModuleLoad (module.js:446:12)
    at Function.Module._load (module.js:438:3)

which has nothing to do with my handler.js file nor with a missing dependency at /var/task/node_modules/github/lib/index.js:5:23.

npm install -g serverless@1.17.0 fixed it for me.

[pmuens]

Hey @maikokuppe thanks for commenting 👍

We just merged the fix from @indieisaconcept this morning. So it should be fixed in master. Could you try the recent master and see if it resolves your issue? Thanks in advance!

[maikokuppe]

@pmuens Tried it with the recent master commit d2e1ef8 but no success. I guess it isn't related to this issue.
I'm going to open a seperate issue if it doesn't resolve in the near future (some other 1.18.0 issues are currently being worked on).

[pmuens]

@pmuens Tried it with the recent master commit d2e1ef8 but no success. I guess it isn't related to this issue.

Thanks of trying it out @maikokuppe 👍

I'm going to open a seperate issue if it doesn't resolve in the near future (some other 1.18.0 issues are currently being worked on).

That sounds like a good idea. We'll release a patch release soon (1.18.1) and 1.19.0 will be released next week. Maybe you can try those versions and open up a new issue if the problem still persists.