Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Extend js file import functionality for serverless.yaml #3378

Closed

Conversation

lteacher
Copy link

What did you implement:

Hello,

I had some free time so I implemented the feature that I requested in #3372. It was a trivial change so figured I would save time and get the PR open to review.

Thanks!

How did you implement it:

Changes to the logic in the Variables.js. Is trivial, should be clear.

How can we verify it:

Added tests. Manual testing involves referencing an existing .js module that returns some useful object.

Todos:

  • Write tests
  • Write documentation - minor change only...
  • Fix linting errors
  • Make sure code coverage hasn't dropped
  • Provide verification config / commands / resources
  • Enable "Allow edits from maintainers" for this PR
  • Update the messages below

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

@eahefnawy
Copy link
Member

@lteacher the reason why we only support function export is to reduce the technical debt on our side without restricting the user on a certain exported data type. If we merge this PR, then we'd also have to support other data types as well (arrays, strings...etc). That would be pretty hard to maintain. We just think a function is the "Standard" datatype.

Hope you understand, just didn't want you waiting for too long on this PR 😊

@eahefnawy eahefnawy closed this Mar 17, 2017
@lteacher
Copy link
Author

@eahefnawy thanks for the fast reply.

Just before I give up on this feature, I would like to mention a few points which in my mind are relevant here to your response.

...the reason why we only support function export is to reduce the technical debt on our side without restricting the user on a certain exported data type

Just want to understand some part of this comment. Are you saying just that you don't want to support in some way the just the precise type that is returned? The reason I am wondering is because I don't really get this. The user can import a function, which returns any data type. So long as they then havent named a property it works. For example, your tests import a function which returns a string which you now support. In my case for a plugin I have written, I import a function which returns the require of the webpack config. It seems like here you still support it but the user just has less flexibility.

Just above the js documentation, we can import an entire .yml config so it seems importing a whole file, in non-js scenarios is accepted.

Also, I actually kind of think that object feels like more the "Standard" data type. The reason I say this is that you actually require the user to specify an object, then using the : separator the name of a property in an object. This property must be a function. After specifically resolving this function, you allow the user to specify '.' notation syntax to reference further object properties if they exist.

Actually, function seems to be the special case in reality where we know that it needs to be resolved. This PR actually supports objects or functions as a result, including resolving functions through the evaluation of the 'deep' referenced properties. The resulting variable, is of any type which you support.

Anyways, just wanted to let you know my thoughts about that. Thanks again for your reply.

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.

2 participants