Skip to content

Conversation

@rehos
Copy link

@rehos rehos commented Jun 14, 2020

The utility functions parseS3 and deepMerge are using this. Both functions are arrow functions, so this is undefined. This pull request fixes both functions.

@coveralls
Copy link

Pull Request Test Coverage Report for Build 281

  • 5 of 5 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.0006%) to 99.614%

Totals Coverage Status
Change from base Build 277: 0.0006%
Covered Lines: 596
Relevant Lines: 597

💛 - Coveralls

@Sleavely
Copy link
Contributor

Are you sure it's undefined? this should be referring to the module scope, i.e. exports. Arrow functions don't have a this, so the scope remains that of the parent where the function was defined, in this case the closest parent is the module.

As an example, lets create and run this-module-scope.js:

exports.potato = () => console.log('potato() was called')

// Lets define and run an arrow function
;(() => {
  this.potato()
})()

@rehos
Copy link
Author

rehos commented Feb 22, 2021

We use rollup in our build process and without the changes of this pull request it generates the following code (notice the explicit undefined keywords:

// Parse S3 path
var parseS3 = path => {
  if (!undefined.isS3(path)) throw new FileError$1('Invalid S3 path',{path})
  let s3object = path.replace(/^s3:\/\//i,'').split('/');
  return { Bucket: s3object.shift(), Key: s3object.join('/') }
};


// Deep Merge
var deepMerge = (a,b) => {
  Object.keys(b).forEach(key => (key in a) ?
    undefined.deepMerge(a[key],b[key]) : Object.assign(a,b) );
  return a
};

And with the changes in this pull request it generates:

// Parse S3 path
var parseS3 = path => {
    if (!isS3(path)) throw new FileError$1('Invalid S3 path', { path })
    let s3object = path.replace(/^s3:\/\//i, '').split('/');
    return { Bucket: s3object.shift(), Key: s3object.join('/') }
};


// Deep Merge
const deepMerge = (a, b) => {
    Object.keys(b).forEach(key => (key in a) ?
        deepMerge(a[key], b[key]) : Object.assign(a, b));
    return a
};

Rollup version used is 2.26.13. I didn't check with a newer versions, but I think they do the same because arrow functions take this from their lexical scope.

Also note that for extractRoutes the current lambda-api library already uses the same construct.

@Sleavely
Copy link
Contributor

I've never used rollup but it looks like you can configure it to set the correct context for NodeJS modules by defaulting the context option to exports, or on a per-file basis with moduleContext as described here: rollup/rollup#1023

As an aside, how come you're transpiling the code at all? The AWS Lambda NodeJS runtimes are fairly modern.

@rehos
Copy link
Author

rehos commented Feb 22, 2021

Thanks for pointing me to the moduleContext configuration option, I totally forgot about that one. In the early days of lambda we really needed the transpilation. At the moment the benefits of rollup are threeshaking - reducing the size of the lambda code. But is is time to run some benchmarks again.

I will close this pull request.

@rehos rehos closed this Feb 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants