Add the concept of lifecycle hooks

[achew22]

Also migrate livereload into being the first consumer of these lifecycle
hooks. This should allow the core of iBazel to be relatively simple but
additional modules be made available to the end user in any boolean
combination.

I'm open and supportive of adding more hooks into the lifecycle of an
iBazel session. If there is something you would like to see hooked, add
it to the Lifecycle interface with a before/after version.

[gregmagolan]

@achew22 I like it. The profiler in #87 could be wired up to this concept with some modifications. The complications that I'd have with this as is are:

1. I'm creating a profile event if and when the livereload is triggered which would require lifecycle events to be able to create new lifecycle events.

2. Change detection events pass the name of the change which wouldn't fit in the beforeEvent() afterEvent() pattern.

I think a few changes could make this more flexible. With different lifecycle event types requiring different parameters and the interface being the natural place to define those parameters, the event types and parameters could be defined by the lifecycle interface as such:

type Lifecycle interface {
	// Initialize is called when iBazel starts
	Initialize()

	// RunTarget is called when a target is being setup to run
	RunTarget(target string, rule *blaze_query.Rule)

	// ChangeDetected is called when a change is detected
	// changeType: "source"|"graph"
	// name: name of source file or build graph change
	ChangeDetected(changeType string, name string)

	// Called before/after running a command
	// commandType: "build"|"test"|"run"
	// success: true when command succeeded, false when failed
	BeforeCommand(commandType string)
	AfterCommand(commandType string, success bool)

	// Called after a livereload is triggered
	ReloadTriggered()

	// Cleanup is called when iBazel is being shutdown
	Cleanup()

For triggering a ReloadTriggered() event from within an AfterCommand() event, a Lifecycle interface could potentially be passed into to each event which could then be used to create other events as such:

AfterCommand(lifecycle Lifecycle, commandType string, success bool)

Thoughts?

[ittaiz]

Andres, With respect to #18 (buildozer auto apply) do you think it should be a hook or is this not a relevant use case for these hooks?

…

On Fri, 8 Dec 2017 at 9:44 Greg Magolan ***@***.***> wrote: @achew22 <https://github.com/achew22> I like it. The profiler in #87 <#87> could be wired up to this concept with some modifications. The complications that I'd have with this as is are: 1. I'm creating a profile event if and when the livereload is triggered which would require lifecycle events to be able to create new lifecycle events. 2. Change detection events pass the name of the change which wouldn't fit in the beforeEvent() afterEvent() pattern. I think a few changes could make this more flexible. With different lifecycle event types requiring different parameters and the interface being the natural place to define those parameters, the event types and parameters could be defined by the lifecycle interface as such: type Lifecycle interface { // Initialize is called when iBazel starts Initialize() // RunTarget is called when a target is being setup to run RunTarget(rule *blaze_query.Rule) // ChangeDetected is called when a change is detected // changeType: "source"|"graph" // name: name of source file or build graph change ChangeDetected(changeType string, name string) // Called before/after running a command // commandType: "build"|"test"|"run" // success: true when command succeeded, false when failed BeforeCommand(commandType string) AfterCommand(commandType string, success bool) // Called after a livereload is triggered ReloadTriggered() // Cleanup is called when iBazel is being shutdown Cleanup() For triggering a ReloadTriggered() event from within an AfterCommand() event, a Lifecycle interface could potentially be passed into to each event which could then be used to create other events as such: AfterCommand(lifecycle Lifecycle, commandType string, success bool) Thoughts? — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#92 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABUIFzxhPPlwRLRTQRdwLbjfc0T4ZiXFks5s-OjGgaJpZM4Q6qV8> .

[mrmeku]

Looks good! I think you just might want to touch up the documentation for interacting with lifecycle hooks so that people other than yourself know the full power of them

[mrmeku]

Can you leave a comment as to why its not necessary to kill the reload server and unset the IBAZEL_LIVERELOAD_URL within the cleanup step? I didn't expect this method to be empty

[achew22]

Good eyes. I messed that one up real good!

[mrmeku]

Seems like you should pass a bit more context. For example, if the live reload server wanted to make an optimization by rereading only changed files, it would need to know which build targets were affected by the build event. Or if I'm missing something maybe you could add a comment to explain how to hook into metadata like that

[achew22]

In the particular case of the live reload server, there can only be one target being run at a time. As a result, in the particular case of live reload the answer to "what target/files should I reload" is always going to be all of them.

There may, however. be some special logic that is useful in a ibazel test scenario. Lemme think on that.

[mrmeku]

Should target decider return a bool of whether it wants to receive events? Otherwise each lifecycle listener will need to maintain state and have logic to perform a noop on BeforeEvent and AfterEvent calls

[achew22]

Yes, that seems more correct than this. I think that might get a little complicated tracking that in a generic form so I'm going to return to it a little later. Todo added with context of why I don't wanna do it right now.

[achew22]

@gregmagolan

I'm creating a profile event if and when the livereload is triggered which would require lifecycle events to be able to create new lifecycle events.

Live reload is currently triggered by an event. Could we have have the profiler note the time when they received the before/after event and say "that's how long it took"? The profiler may be a special case where we add some profiling code into one of the lifecycle listeners, but I'd like to avoid it if possible.

Change detection events pass the name of the change which wouldn't fit in the beforeEvent() afterEvent() pattern.

Thanks for the feedback! I'll be updating my commit in just a second to reflect your desired changes. One of the nice things about this interface is that all the implementors are going to be in the same repo so if we want to make changes we still can. In addition to the success information I would like to pass the action's run output. It might be possible to do some nifty things with the test output where you print out a reformatted form of the test output that's easier to read. Or, in the case of a build failure, the liveserver pushes a message to the browser that says "Hey, you had a typo on line 12 of foo.ts and your build didn't work".

@ittaiz, that's one of the many things I think we can solve with this. We can write a relatively simple post execution hook that will get the blaze $COMMAND output buffer after the program exits, parse it for magic strings, and then invoke bazel run //:gazelle or buildozer add ... or whatever it is you think should be run afterward.

@mrmeku, thanks for the review. Really good comments which I will address individually when I upload more code in a little bit.

[ittaiz]

Awesome! I'll try to take it as a motivation for picking the buildozer feature after this is merged

…

On Sat, 9 Dec 2017 at 8:45 Andrew Z Allen ***@***.***> wrote: @gregmagolan <https://github.com/gregmagolan> I'm creating a profile event if and when the livereload is triggered which would require lifecycle events to be able to create new lifecycle events. Live reload is currently triggered by an event. Could we have have the profiler note the time when they received the before/after event and say "that's how long it took"? The profiler may be a special case where we add some profiling code into one of the lifecycle listeners, but I'd like to avoid it if possible. Change detection events pass the name of the change which wouldn't fit in the beforeEvent() afterEvent() pattern. Thanks for the feedback! I'll be updating my commit in just a second to reflect your desired changes. One of the nice things about this interface is that all the implementors are going to be in the same repo so if we want to make changes we still can. In addition to the success information I would like to pass the action's run output. It might be possible to do some nifty things with the test output where you print out a reformatted form of the test output that's easier to read. Or, in the case of a build failure, the liveserver pushes a message to the browser that says "Hey, you had a typo on line 12 of foo.ts and your build didn't work". @ittaiz <https://github.com/ittaiz>, that's one of the many things I think we can solve with this. We can write a relatively simple post execution hook that will get the blaze $COMMAND output buffer after the program exits, parse it for magic strings, and then invoke bazel run //:gazelle or buildozer add ... or whatever it is you think should be run afterward. @mrmeku <https://github.com/mrmeku>, thanks for the review. Really good comments which I will address individually when I upload more code in a little bit. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#92 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABUIF9o3JrFhBArAIEzrhNDcH2-osRP8ks5s-iyDgaJpZM4Q6qV8> .