Make new-style script objects a bundle.

[hawkinsw]

With this, a user will be able to store in a single file
a new-style script object that bundles the entirety of the
functionality necessary to prepare the browser to run a
test, collect the generated data and restore the browser
to its previous state.

[ncalexan]

So I can see where this is going, but I have some high-level questions to discuss before nailing down the specifics.

1. Fundamentally I still don't understand why the new pre- and post-scripts look so different to the existing pre- and post-scripts. We talked about this before we split out the first part of the work, and it's not clarified here. To whit: this is all possible right now with a --{pre,post}Script -- it's just a little hidden 'cuz of the new JS execution APIs. It's very awkward to transition between a regular and a new-style pre-script, say, 'cuz the APIs are different. Can we harmonize this?

2. This doesn't handle the most fundamental "pre" condition, which is whether a particular script is wanted. Let's make the pre-script return a boolean, allowing it to inspect the environment/command line (see the API changes requested above) in order to know whether or not to run it. (Without this we can't add new "feature-like" measurements that are opt-in.)

What happens if a pre- or post- script raises an error? We shouldn't run the measurement collection if we couldn't set up the pre-conditions.

[ncalexan]

Minimize these white space changes, please. Every one is a merge conflict waiting to happen.

[ncalexan]

Here and many places. They're not only merge conflicts, they take time to review and process.

[ncalexan]

This is very horizontally narrow. Can we please use our large monitors? We don't have infinite vertical space!

[hawkinsw]

eslint (correctly, imo!) says that we do not want lines longer than 80 characters, the standard terminal size. Because this line will end up being longer than 80 characters, eslint seems to aggressively add new lines between each of the parameters and line them up vertically. What do you suggest as the maximum line length? I can change that and commit it to the eslint rules in the repo so that this is not a problem in the future.

[ncalexan]

Can we have an example (or a new script?) with non-trivial pre and post?

[hawkinsw]

There is an example included in the docs/ directory. Would you like me to commit it to the repo?

[ncalexan]

Yes, I want there to be an example (command-line-option-ed off) that's easy for everybody to run/copy.

[ncalexan]

Why is this a problem now? Can we avoid this whole business by having generateScriptObject do whatever it would have done before?

[ncalexan]

Do we think that order matters? Should we try to execute like:

pre-A
pre-B
A
B
post-B
post-A

[hawkinsw]

So I can see where this is going, but I have some high-level questions to discuss before nailing down the specifics.

1. Fundamentally I still don't understand why the new pre- and post-scripts look so different to the existing pre- and post-scripts.  We talked about this before we split out the first part of the work, and it's not clarified here.  To whit: this is all possible _right now_ with a `--{pre,post}Script` -- it's just a little hidden 'cuz of the new JS execution APIs.  It's very awkward to transition between a regular and a new-style pre-script, say, 'cuz the APIs are different.  Can we harmonize this?

As we discussed on our call today, I believe that there are two fundamental reasons for consolidating pre/post + collection into a single bundle:

1. Documentation
   Having the setup/teardown code associated with the measurement/collection code makes it easier for the initial developer and subsequent users/maintainers/extenders to a) understand that all the pieces must go together for the script to work and b) see how the pieces work together. If there is one piece of the action in a prescript file and another in a measurement script file and another in the postscript file, it's hard to keep track of how the developer conceived of those working together and their dependencies.

2. Development
   The pre/post script environment are fundamentally different than the script environment. The script environment executes that contents of its script in the context of the browser under test whereas the pre/post script environment executes the contents of its script in a node context with access to the context of the browser under test. Keeping these straight may strain the developer. Yes, there are things that the pre/post script will want to do that do not need to be done in the context of the browser (see below), but I think that most of the time the pre/post actions will relate to the state of the browser under test.

2. This doesn't handle the most fundamental "pre" condition, which is whether a particular script is wanted.  Let's make the pre-script return a boolean, allowing it to inspect the environment/command line (see the API changes requested above) in order to know whether or not to run it.  (Without this we can't add new "feature-like" measurements that are opt-in.)

This is a very good critique and suggestion. I agree that the pre script should be expected to return a boolean that will signal whether the measurement should take place. To answer the question of, "How does the developer use the pre function to tell browsertime that this feature is optional and the browsertime user does not want its measurement executed?" does this sound reasonable: The pre script will take a(n) (optional) parameter to the pre script which will be an array of the CLI options from the invocation of browsertime that browsertime did not consume for itself (in other words, the ones that it didn't recognize). The script can iterate through those CLI options and decide whether or not to execute. If it needs to execute, it does the required setup methods and returns true. If those required setup methods cannot be executed, it can return false and the measurement will not run. Alternatively, the script can simply return false based on the values from the CLI, indicating that the browsertime user does not want the measurement executed.

What do you think about this?

What happens if a pre- or post- script raises an error? We shouldn't run the measurement collection if we couldn't set up the pre-conditions.

See above.

[ncalexan]

So I can see where this is going, but I have some high-level questions to discuss before nailing down the specifics.

1. Fundamentally I still don't understand why the new pre- and post-scripts look so different to the existing pre- and post-scripts.  We talked about this before we split out the first part of the work, and it's not clarified here.  To whit: this is all possible _right now_ with a `--{pre,post}Script` -- it's just a little hidden 'cuz of the new JS execution APIs.  It's very awkward to transition between a regular and a new-style pre-script, say, 'cuz the APIs are different.  Can we harmonize this?

As we discussed on our call today, I believe that there are two fundamental reasons for consolidating pre/post + collection into a single bundle:

1. Documentation
   Having the setup/teardown code associated with the measurement/collection code makes it easier for the initial developer and subsequent users/maintainers/extenders to a) understand that all the pieces must go together for the script to work and b) see how the pieces work together. If there is one piece of the action in a prescript file and another in a measurement script file and another in the postscript file, it's hard to keep track of how the developer conceived of those working together and their dependencies.

You can still put the pre-script in the same file, keeping the code close together. You're already require-ing the measurements, which is how pre/post scripts as written now work.

2. Development
   The pre/post script environment are fundamentally different than the script environment. The script environment executes that contents of its script in the context of the browser under test whereas the pre/post script environment executes the contents of its script in a `node` context _with access to_ the context of the browser under test. Keeping these straight may strain the developer. Yes, there are things that the pre/post script will want to do that do not need to be done in the context of the browser (see below), but I think that most of the time the pre/post actions will relate to the state of the browser under test.

Sure, it'll strain the developer. We're adding a general mechanism for adding features to browsertime; that's not easy. This way you can define a pre-condition (flags, whatever else), take the measurement, and post-process it -- potentially doing things like pulling files off an Android device, etc. That doesn't make sense in a browser context and never will.

I trust developers to understand these different contexts (they already have to for pre/post scripts and measurements).

2. This doesn't handle the most fundamental "pre" condition, which is whether a particular script is wanted.  Let's make the pre-script return a boolean, allowing it to inspect the environment/command line (see the API changes requested above) in order to know whether or not to run it.  (Without this we can't add new "feature-like" measurements that are opt-in.)

This is a very good critique and suggestion. I agree that the pre script should be expected to return a boolean that will signal whether the measurement should take place. To answer the question of, "How does the developer use the pre function to tell browsertime that this feature is optional and the browsertime user does not want its measurement executed?" does this sound reasonable: The pre script will take a(n) (optional) parameter to the pre script which will be an array of the CLI options from the invocation of browsertime that browsertime did not consume for itself (in other words, the ones that it didn't recognize). The script can iterate through those CLI options and decide whether or not to execute. If it needs to execute, it does the required setup methods and returns true. If those required setup methods cannot be executed, it can return false and the measurement will not run. Alternatively, the script can simply return false based on the values from the CLI, indicating that the browsertime user does not want the measurement executed.

What do you think about this?

This seems very specific when there's a general mechanism for doing this and more already.

[hawkinsw]

So I can see where this is going, but I have some high-level questions to discuss before nailing down the specifics.

1. Fundamentally I still don't understand why the new pre- and post-scripts look so different to the existing pre- and post-scripts.  We talked about this before we split out the first part of the work, and it's not clarified here.  To whit: this is all possible _right now_ with a `--{pre,post}Script` -- it's just a little hidden 'cuz of the new JS execution APIs.  It's very awkward to transition between a regular and a new-style pre-script, say, 'cuz the APIs are different.  Can we harmonize this?

As we discussed on our call today, I believe that there are two fundamental reasons for consolidating pre/post + collection into a single bundle:

1. Documentation
   Having the setup/teardown code associated with the measurement/collection code makes it easier for the initial developer and subsequent users/maintainers/extenders to a) understand that all the pieces must go together for the script to work and b) see how the pieces work together. If there is one piece of the action in a prescript file and another in a measurement script file and another in the postscript file, it's hard to keep track of how the developer conceived of those working together and their dependencies.

You can still put the pre-script in the same file, keeping the code close together. You're already require-ing the measurements, which is how pre/post scripts as written now work.

2. Development
   The pre/post script environment are fundamentally different than the script environment. The script environment executes that contents of its script in the context of the browser under test whereas the pre/post script environment executes the contents of its script in a `node` context _with access to_ the context of the browser under test. Keeping these straight may strain the developer. Yes, there are things that the pre/post script will want to do that do not need to be done in the context of the browser (see below), but I think that most of the time the pre/post actions will relate to the state of the browser under test.

Sure, it'll strain the developer. We're adding a general mechanism for adding features to browsertime; that's not easy. This way you can define a pre-condition (flags, whatever else), take the measurement, and post-process it -- potentially doing things like pulling files off an Android device, etc. That doesn't make sense in a browser context and never will.

I trust developers to understand these different contexts (they already have to for pre/post scripts and measurements).

2. This doesn't handle the most fundamental "pre" condition, which is whether a particular script is wanted.  Let's make the pre-script return a boolean, allowing it to inspect the environment/command line (see the API changes requested above) in order to know whether or not to run it.  (Without this we can't add new "feature-like" measurements that are opt-in.)

This is a very good critique and suggestion. I agree that the pre script should be expected to return a boolean that will signal whether the measurement should take place. To answer the question of, "How does the developer use the pre function to tell browsertime that this feature is optional and the browsertime user does not want its measurement executed?" does this sound reasonable: The pre script will take a(n) (optional) parameter to the pre script which will be an array of the CLI options from the invocation of browsertime that browsertime did not consume for itself (in other words, the ones that it didn't recognize). The script can iterate through those CLI options and decide whether or not to execute. If it needs to execute, it does the required setup methods and returns true. If those required setup methods cannot be executed, it can return false and the measurement will not run. Alternatively, the script can simply return false based on the values from the CLI, indicating that the browsertime user does not want the measurement executed.
What do you think about this?

This seems very specific when there's a general mechanism for doing this and more already.

You are advocating, then, for having the pre/post functions in a bundle with the collect function, executing pre/post functions in node context and taking the same parameters as they do now, and executing the collect function in the context of the browser?

pre: takes two parameters (context and commands) and operates in a node context.
collect: takes no parameters (context and commands) and operates in the browser context.
post: takes two parameters (context and commands) and operates in a node context.

If we are being expansive, what would you think about having all three take the same parameters and executing in the node context? Since you can always reach into the browser context and execute a function (using the context parameter), this won't limit flexibility. In fact, it would add flexibility by giving the developer the opportunity to collect statistics external to the object under test (e.g., network traffic, energy consumption, etc).

pre: takes two parameters (context and commands) and operates in a node context.
collect: takes two parameters (context and commands) and operates in the node context.
post: takes two parameters (context and commands) and operates in a node context.

[ncalexan]

First, sorry for the delayed reply.

You are advocating, then, for having the pre/post functions in a bundle with the collect function, executing pre/post functions in node context and taking the same parameters as they do now, and executing the collect function in the context of the browser?

pre: takes two parameters (context and commands) and operates in a node context.
collect: takes no parameters (context and commands) and operates in the browser context.
post: takes two parameters (context and commands) and operates in a node context.

Yes.

If we are being expansive, what would you think about having all three take the same parameters and executing in the node context? Since you can always reach into the browser context and execute a function (using the context parameter), this won't limit flexibility. In fact, it would add flexibility by giving the developer the opportunity to collect statistics external to the object under test (e.g., network traffic, energy consumption, etc).

pre: takes two parameters (context and commands) and operates in a node context.
collect: takes two parameters (context and commands) and operates in the node context.
post: takes two parameters (context and commands) and operates in a node context.

I'm all for this. In fact, I'm embarrassed not to have counselled this the first time around. When I first started using browsertime I spent some time digging into the mechanics of result collection and was disappointed that I could not get at results for new measurements from node context scripts. This approach would address this.

One big advantage with this would be that it generalizes smoothly to other browsers (e.g., Chrome); and in fact it subsumes existing browsertime features like HAR collection and log/trace collection.

Can you work this up, and see if the changes to the AppConstants example feel good?