search

serverless / template

Compose & provision a collection of Serverless Components
https://serverless.com
Apache License 2.0
10 stars 6 forks source link

Fix programmatic method invocation. #9

Closed Pavel910 closed 4 years ago

Pavel910 commented 4 years ago

@eahefnawy this PR contains a small fix that was breaking the programmatic invocation of custom methods on the @serverless/template component. CLI usage however was working normally.

I also added a README file with some examples as I see a lot of people struggling with programmatic usage of this component.

This has been tested manually for both CLI and programmatic usage, all the cases I could think of.

barrysteyn commented 4 years ago

@eahefnawy This is fixing a breaking change, so it is super important to be addressed. @Pavel910 Can I make the following comments:

  1. Throwing an error in createCustomMethod may be appropriate, but without any testing, changes like this can (and in fact, this change does) cause unintended consequences.
  2. Perhaps unit/integration tests on the core libraries. Not saying that things need to be tested vigorously but it may be a good idea (in fact, I will open an issue for this).
  3. Thanks very much for the README. It is super helpful.

EDIT: Here is issue for testing https://github.com/serverless/components/issues/511

Pavel910 commented 4 years ago

@barrysteyn throwing there is ok because there is nothing else that can be executed if there is no custom method (read my comment in the review section).

To make this component better overall, the template path should be passed to constructor, and not to methods themselves (then your proposed solution with trying would work nicely), but then it is a major change that also involves updates to CLI. That's something @eahefnawy must allow us to do :)

barrysteyn commented 4 years ago

To make this component better overall, the template path should be passed to constructor, and not to methods themselves (then your proposed solution with trying would work nicely), but then it is a major change that also involves updates to CLI. That's something @eahefnawy must allow us to do :)

It seems like this may be the way to go. However such a big change may need to wait for testing (sorry I am going on about testing, but it seems tough to make any change to a core library without testing).

Pavel910 commented 4 years ago

@barrysteyn Agreed, it would be nice to have at least basic tests in place.

eahefnawy commented 4 years ago

Thanks folks. Testing is absolutely planned. We just didn't 100% settle on the API yet and currently taking feedback. PR like these are exactly the feedback that we need.

LG2M 😊