-
Notifications
You must be signed in to change notification settings - Fork 161
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added some feedback.
b3eefb7
to
6d3be0f
Compare
d2ba8b8
to
8edcb89
Compare
src/services/funcService.ts
Outdated
} | ||
|
||
private getServerlessYml() { | ||
return this.serverless.utils.readFileSync("serverless.yml"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does the framework have a way to get the name of the yaml file? With the new --config
option, it's possible that the file won't be named serverless.yml
. We don't have to over-index around this, but just something to keep in mind.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. Wasn't accounting for this. I'll investigate further here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a getter that defaults to serverless.yml
. Good callout
private getFunctionHandler(name: string) { | ||
return `"use strict"; | ||
|
||
module.exports.handler = async function (context, req) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I almost want to suggest making this a separate template/file (also easier to tell if it's valid code), but given that it's encapsulated here and relatively short, I'm okay with it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd love to capture this in a template file, and we actually discussed this in a previous PR (previous PR got a little murky with all the refactoring, so I closed and opened a new one). The issue I ran into was deciding what kind of file to store the template in:
1. .ts - transpiles to messy .js in build
2. .js - can't be included with current tsconfig (`allowJs` and `declarations` attribute cannot both be true)
3. .txt - Would need to copy to file post-build to the lib directory - seems heavy handed for this ask
If you have any other thoughts here, I'm open to any and all suggestions :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, good call. I was thinking some way of reading from a file, but it gets messy in the context of the rest of the build. Will keep thinking about it -- would be nice to have separate, but we can call it good until something else better presents itself.
Looking good - just a couple comments. Great work on the tests. 👍 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
some comments
// Trivial test for now. In the future, this process | ||
// may spawn the start process itself rather than telling | ||
// the user how to do it. | ||
expect(sls.cli.log).toBeCalledTimes(3); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have assumed that this was the original plan. Are we holding off on it now because we want to get something usable, albeit manual, out?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, holding off on the spawn for now. Hoping npm start
is not too much work for the time being
ce49a7f
to
1704787
Compare
AzureOfflinePlugin
OfflineService
that contains the logicUpdate:
func
plugin to account for new package structureFuncService
to contain logic and utility functions for plugin