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

Fix command resolution when watch is involved #1004

Merged

Conversation

medikoo
Copy link
Contributor

@medikoo medikoo commented Nov 19, 2021

When testing the plugin with the Serverless Framework, I've discovered that the plugin finalizes the given processing hook (and in turn command) while leaving some command-specific logic working in the background.

Such configuration introduces some erroneous side effects. Framework assumes that command was finalized and pursues some cleanup operations. Command seems to operate without issues only because Framework doesn't imply a hard exit on the process.

It's especially important to have it fixed before v3 of the Framework is released, as it comes with new logging interfaces, and e.g. on command finalization, all progress bars are permanently cleared, leaving this feature no further operable (while it's a valuable feature for watching as configured in this plugin)

@medikoo medikoo changed the title Fix watch command resolution Fix command resolution when watch is involved Nov 19, 2021
Copy link
Member

@j0k3r j0k3r left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be honest, I never used the --watch option.
But this seems fair, thanks for submitting the PR ahead of Serverless v3.
Could you just rebase against the master so the branch will be up to date? Thanks

@j0k3r j0k3r added this to the 5.6.0 milestone Nov 19, 2021
@medikoo medikoo force-pushed the 1119-fix-processing-resolution branch from 179d24f to 19712f1 Compare November 19, 2021 14:59
@medikoo
Copy link
Contributor Author

medikoo commented Nov 19, 2021

Could you just rebase against the master so the branch will be up to date? Thanks

Sure, rebased

Copy link
Member

@vicary vicary left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Never successfully setup the --watch option either, whenever I tried serverless invoke local --watch it always fails to run upon code change.

image

Despite the error above, a quick check with serverless@^3.0.0-pre proves this change indeed prevents a crash.

I am happy to take this at its current state, in fact any improvements are welcomed.

@j0k3r
Copy link
Member

j0k3r commented Nov 19, 2021

@medikoo could again rebase against the master? Thanks

@medikoo medikoo force-pushed the 1119-fix-processing-resolution branch from 19712f1 to 37d9e0f Compare November 19, 2021 16:19
@medikoo
Copy link
Contributor Author

medikoo commented Nov 19, 2021

Sure, pushed

@j0k3r j0k3r merged commit ca5c77b into serverless-heaven:master Nov 19, 2021
@j0k3r j0k3r mentioned this pull request Nov 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