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

Integrate with "Serverless invoke local" and change the "webpack invoke" command #151

Closed
HyperBrain opened this Issue Jul 11, 2017 · 8 comments

Comments

Projects
None yet
3 participants
@HyperBrain
Copy link
Member

HyperBrain commented Jul 11, 2017

This is a Feature Proposal

Description

Serverless offers a fully working local invoke command that handles all possible
kinds of invocations, sets the function's environment correctly and much more.

There is no need to have a separate webpack invoke command that replicates (only)
parts of this functionality and binds resources to maintain it properly.

It would be better to integrate seamlessly with local invoke by hooking the important
lifecycle events exposed by the command. Finally, serverless invoke local should
invoke the function after the webpack compile has been triggered as part of the lifecycle
hooks in place.

To make it completely seamless, the webpack invoke command should techincally just use Serverless'
pluginManager to spawn the invoke local command like this: this.serverless.pluginManager.spawn('invoke:local').

This solution would also make the invocation provider independent as Serverless fully takes care of that already.

Related issue(s):
#128 (arbitrary provider support)

@HyperBrain HyperBrain changed the title Integrate with "Serverless invoke local" and remove the "webpack invoke" command Integrate with "Serverless invoke local" and change the "webpack invoke" command Jul 11, 2017

@HyperBrain HyperBrain added the feature label Jul 12, 2017

@HyperBrain HyperBrain added this to the 3.0.0 milestone Jul 12, 2017

@HyperBrain HyperBrain referenced this issue Jul 14, 2017

Merged

Integrate with "serverless invoke local" #153

5 of 5 tasks complete

@HyperBrain HyperBrain self-assigned this Jul 14, 2017

@HyperBrain

This comment has been minimized.

Copy link
Member Author

HyperBrain commented Jul 14, 2017

During the implementation I saw that it could make sense to couple the watch command with the serverless invoke local too, so that Serverless takes care of setting the environment and other stuff automatically in that case.
I tend to change that within the context of the attached PR too.
Are there any objections?

@HyperBrain

This comment has been minimized.

Copy link
Member Author

HyperBrain commented Jul 14, 2017

@benjaminwood @hassankhan The basic implementation is finished (support of "serverless invoke local"), i.e. sls invoke local should now work with the plugin. I did a few tests with my test project. Could you also verify that it works with this PR (#153)?

I will finish it over the weekend and remove the now obsolete code, adapt the docs and write unit tests.

@hassankhan

This comment has been minimized.

Copy link
Contributor

hassankhan commented Jul 14, 2017

I'm away on holiday for the weekend but I'll try and give it a go, seems simple enough to test 😄

@benjaminwood

This comment has been minimized.

Copy link
Contributor

benjaminwood commented Jul 15, 2017

This is great, thanks @HyperBrain! It does work for me, though I did run into one snag.

When using the invoke local command with the --path (-p) flag, relative file paths will not be found because serverless will end up looking in the build directory (.webpack) later on.

I took a stab at fixing this and I'm happy to PR it if I'm headed down the right path: benjaminwood@37086ed

Couldn't think of a good name for the function. Also, I'm not sure that this should go in the utils file. Happy to make revisions! Let me know what you think.

@HyperBrain

This comment has been minimized.

Copy link
Member Author

HyperBrain commented Jul 15, 2017

Hey @benjaminwood . Thanks a lot, good catch 👍 . In general, it is better to open a PR instead of linking a commit - regardless if the approach is feasible or not. It's just easier to discuss a possible solution/implementation within a PR in GitHub - as easy as it is to merge or revoke the PR later. This will keep the main issue clean (the PR candidates are still listed if they are linked correctly) and put the implementation specific discussion near to the implementation itself.

So just open a PR 😃 - I added templates for PRs recently, so the PR description will additionally contain important info (e.g. if it is ready for review/discussion, etc.). Target the PR to the v3.0.0-invoke-local branch.

... and I'm really glad to hear that my changes work (with exception of the path issue)

@HyperBrain

This comment has been minimized.

Copy link
Member Author

HyperBrain commented Jul 15, 2017

@hassankhan No hurry, enjoy your holidays 😃

@benjaminwood

This comment has been minimized.

Copy link
Contributor

benjaminwood commented Jul 15, 2017

Thanks @HyperBrain I've opened a PR: #156

@HyperBrain

This comment has been minimized.

Copy link
Member Author

HyperBrain commented Jul 15, 2017

@benjaminwood Cool 🙌 . I commented there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment