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

Introduce script packs as script. #238

Closed
glennblock opened this issue May 8, 2013 · 32 comments
Closed

Introduce script packs as script. #238

glennblock opened this issue May 8, 2013 · 32 comments

Comments

@glennblock
Copy link
Contributor

Yes we're going there. This will enable to you share scripts in nuget packages as script modules.

It will work very similar to node. Module scriptcs can depend on other modules. All modules will get installed in a modules folder.

Yes, we do love the node module system!

@ghost ghost assigned glennblock May 8, 2013
@jrusbatch
Copy link
Member

👍

@BrendanThompson
Copy link

👍 this is going to be AWESOME!

@dschenkelman
Copy link
Contributor

Isn't #179 the same? We should close that one.

@jrusbatch
Copy link
Member

@dschenkelman Thanks for pointing that out!

@glennblock
Copy link
Contributor Author

I totally forgot about this one. Need to find some cycles to revisit it. Or maybe someone else takes it.

@glennblock
Copy link
Contributor Author

Starting on this now! (finally!)

How it works

  • Script based script packs are nuget packages with a content folder. Script pack scripts / related scripts are put in that folder.
  • The script pack must end in ScriptPack.csx in the filename. (we could expand this later).
  • You can have other scripts in the same folder which are #loaded.
  • Scriptcs will pick up those scripts and compile them. Ideally this would happen in one single compile, but that may not work out.

Versioning enhancements

My hope is this will introduce some new versioning sXs scenarios since the caller only cares about the name / the type reference is injected at compile time ;-) So you should be able to have (eventually) 2 diff versions of the the same script in the same app!

Authoring the script pack CSX file.

Planning to have 2 modes of authoring:

  • You implement an IScriptPack / IScriptPackContext and just return the type (the non-scripty way).
  • You use a ScripPackBuilder and configure with a fluent API. Still requires implementing a context, but much more compact and easier in script (I'd argue).

Within the script pack

  • You can Require other script packs, just like standard script packs.
  • You can still have assemblies included if you need to.

Being able to requite other script packs is cool because it means those can also be authored in scripts. Thus an ecosystem of script "libraries" becomes possible!

Gist of the syntaxes here: https://gist.github.com/glennblock/8888901

glennblock added a commit to glennblock/scriptcs that referenced this issue Feb 9, 2014
glennblock added a commit to glennblock/scriptcs that referenced this issue Feb 9, 2014
@filipw
Copy link
Member

filipw commented Feb 9, 2014

this is very nice! script packs as scripts are one of the great missing pieces - as they would allow us to be entirely self-contained (no need for VS to extend scriptcs)! I guess the next would be modules as scripts :-D

Some comments:
In the first example I wouldn't use a return, it looks awkward - and we never did this in any scripts. Also "return" statement like that wouldn't compile with Roslyn, return in CSX script is the last statement that evaluates to some result, without a semi colon, placed at the end of the script.
But if the script pack is authored in script by implementing an IScriptPack then we could just compile the CSX, then it's in memory and script packs can be looked up from memory like that.

The second example is very intriguing, I love the direction it's going.
I assume it's convention based? i.e. having the Init method and so on.

One problem I can potentially see with manually wrapping the "script" code in a class though, is that not all of our scripting style would work. For example you could never have "global" variable declarations there, or a Require<T> (I don't think wrapper class would have a require?) since these would break.
On the other hand, I can't think of a viable alternative from the top of my head though, but I've been thinking something along the lines of utilizing a custom host object to help author the script pack (perhaps exposing a script pack builder to the script over a host object method?) and using that and Roslyn to run the script pack as script, and then once compiled, picking up the relevant IScriptPack types from that. Not sure if that would work though, but just thinking out loud. What do you think?

@glennblock
Copy link
Contributor Author

Good point on the return, yes you need to use just typeof to return, but
you are right we can just reflect to find as it will be compiled in.
Actually MEF should just find it as it has an InheritedExport!

As to the Require that we should be able to surface into the outer class
via a static member.

On Sunday, February 9, 2014, Filip W notifications@github.com wrote:

this is very nice! script packs as scripts are one of the great missing
pieces - as they would allow us to be entirely self-contained (no need for
VS to extend scriptcs)! I guess the next would be modules as scripts :-D

Some comments:
In the first example I wouldn't use a return, it looks awkward - and we
never did this in any scripts. Also "return" statement like that wouldn't
compile with Roslyn, return in CSX script is the last statement that
evaluates to some result, without a semi colon, placed at the end of the
script.
But if the script pack is authored in script by implementing an
IScriptPack then we could just compile the CSX, then it's in memory and
script packs can be looked up from memory like that.

The second example is very intriguing, I love the direction it's going.
I assume it's convention based? i.e. having the Init method and so on.

One problem I can potentially see with manually wrapping the "script" code
in a class though, is that not all of our scripting style would work. For
example you could never have "global" variable declarations there, or a
Require (I don't think wrapper class would have a require?) since
these would break.
On the other hand, I can't think of a viable alternative from the top of
my head though, but I've been thinking something along the lines of
utilizing a custom host object to help author the script pack (perhaps
exposing a script pack builder to the script over a host object method?)
and using that and Roslyn to run the script pack as script, and then once
compiled, picking up the relevant IScriptPack types from that. Not sure
if that would work though, but just thinking out loud. What do you think?

Reply to this email directly or view it on GitHubhttps://github.com//issues/238#issuecomment-34575494
.

@glennblock
Copy link
Contributor Author

Yes on modules as scripts. Once we have the infra in place should be easy.
Also CLI as script as well ;-)

On Sunday, February 9, 2014, Glenn Block glenn.block@gmail.com wrote:

Good point on the return, yes you need to use just typeof to return, but
you are right we can just reflect to find as it will be compiled in.
Actually MEF should just find it as it has an InheritedExport!

As to the Require that we should be able to surface into the outer
class via a static member.

On Sunday, February 9, 2014, Filip W <notifications@github.comjavascript:_e(%7B%7D,'cvml','notifications@github.com');>
wrote:

this is very nice! script packs as scripts are one of the great missing
pieces - as they would allow us to be entirely self-contained (no need for
VS to extend scriptcs)! I guess the next would be modules as scripts :-D

Some comments:
In the first example I wouldn't use a return, it looks awkward - and we
never did this in any scripts. Also "return" statement like that wouldn't
compile with Roslyn, return in CSX script is the last statement that
evaluates to some result, without a semi colon, placed at the end of the
script.
But if the script pack is authored in script by implementing an
IScriptPack then we could just compile the CSX, then it's in memory and
script packs can be looked up from memory like that.

The second example is very intriguing, I love the direction it's going.
I assume it's convention based? i.e. having the Init method and so on.

One problem I can potentially see with manually wrapping the "script"
code in a class though, is that not all of our scripting style would work.
For example you could never have "global" variable declarations there, or a
Require (I don't think wrapper class would have a require?) since
these would break.
On the other hand, I can't think of a viable alternative from the top of
my head though, but I've been thinking something along the lines of
utilizing a custom host object to help author the script pack (perhaps
exposing a script pack builder to the script over a host object method?)
and using that and Roslyn to run the script pack as script, and then once
compiled, picking up the relevant IScriptPack types from that. Not sure
if that would work though, but just thinking out loud. What do you think?

Reply to this email directly or view it on GitHubhttps://github.com//issues/238#issuecomment-34575494
.

@glennblock
Copy link
Contributor Author

OK @filipw after messing with this a while, I came up with a solution that can work. Basically we can make an IRequirer which is a dependency, which can be used within the context class. You don't really need it in the main declaration. So let's say I have a context that is relying on other script packs that it needs to access, it would look like this.

public class d910912a3084473395f860c67223210c : ScriptPackTemplate {
  public class DooHickey : IScriptPackContext {
    private ThingAMahiggy _thingamajiggy;

    public DooHickey(IRequirer requirer) {
      _thingamajiggy = requirer.Require<ThingAMajiggy>();
    }
  }
}

@dschenkelman
Copy link
Contributor

@glennblock @filipw what if we change IScriptPackContext to BaseScriptPackContext, add a Require method to it? Would that work (really haven't thought about it)?

@dschenkelman
Copy link
Contributor

@glennblock @filipw BaseScriptPackContext would still use the IRequirer, but Script Pack authors can still use Require instead of requirer.Require.

@glennblock
Copy link
Contributor Author

I wouldn't do that as that would force implementing it. Having the optional
dependency is pretty easy to explain. You already use ctor injection for
other things.

On Sun, Feb 9, 2014 at 1:15 PM, Damian Schenkelman <notifications@github.com

wrote:

@glennblock https://github.com/glennblock @filipwhttps://github.com/filipwwhat if we change
IScriptPackContext to BaseScriptPackContext, add a Require method to it?
Would that work (really haven't thought about it)?

Reply to this email directly or view it on GitHubhttps://github.com//issues/238#issuecomment-34586911
.

@dschenkelman
Copy link
Contributor

@glennblock ok. I thought that since we are kind of forcing IScriptPackContext if we just changed that it would be similar.

@glennblock
Copy link
Contributor Author

That would be a breaking change since all the script packs out there use
IScriptPackContext today.

We could possibly look to have a ScriptPackContext class that implements
IScriptPackContext and adds a Require which invokes a Requirer property
which is injected via Autofac. I'll see what that looks like.

On Sun, Feb 9, 2014 at 1:22 PM, Damian Schenkelman <notifications@github.com

wrote:

@glennblock https://github.com/glennblock ok. I thought that since we
are kind of forcing IScriptPackContext if we just changed that it would
be similar.

Reply to this email directly or view it on GitHubhttps://github.com//issues/238#issuecomment-34587172
.

@glennblock
Copy link
Contributor Author

@dschenkelman something like this could work and would not break existing

    public abstract class ScriptPackContext : IScriptPackContext
    {
        public IRequirer Requirer { get; set; }

        public T Require<T>() where T:IScriptPackContext
        {
            return Requirer.Require<T>();
        }
    }

Now if you derive from this you get Require.

@dschenkelman
Copy link
Contributor

Great! That is what I was referring to with the first option except that you named it ScriptPackContext instead of BaseScriptPackContext which is even better.

Do we really care about breaking changes? I'd rather make the change now before 1.0.0.

@glennblock
Copy link
Contributor Author

I care. We have probably 30 script packs at least that we know about, I don't want them all to break. Also I generally don't want to force people to use our base classes i.e. for testing etc.

@glennblock
Copy link
Contributor Author

I've been doing a ton of work this the past week burning the midnight oil (with a bunch of help from @filipw and @dschenkelman), and I am EXTREMELY happy to tell you that tonight was a breakthrough!!! The first scripted script pack in history IS working :-)

scriptcs

There's still a bunch to do. It needs to be optimized, code consolidated and cleaned up a bit, but you can try it out if you want by grabbing my fork! (Normal disclaimers apply).

Here are the steps:

  1. Clone my fork here: https://github.com/glennblock/scriptcs/tree/238
  2. Build it / make sure scriptcs in your path
  3. Download the zip here: https://dl.dropboxusercontent.com/u/6860088/scripted-script-packs.zip
  4. Go into the test folder and run "scriptcs test.csx".
  5. You'll get a message which is generated from a scripted script pack called ScriptCs.Greeter.
  6. Using scriptcs-install works fine. If you wanna test that add the package (which is in the greeter folder) in VS to be a source.

@filipw
Copy link
Member

filipw commented Feb 20, 2014

This is great.

Some observations after going through the code. Looks like this will trigger quite a few changes across the codebase:

  • we don't need to inject IScriptHostFactory to script engine anymore, all this code can be removed from all variations of the engine, as well as from the tests
  • engine tests will need to be considerably re-written since they all mock/setup IScriptHostFactory now
  • scriptargs array can be removed from the execute signature - it is now completely redundant, it was only used to create the host
  • right now the fluent interface to build script pack needs to be called in this order References then Imports (since imports is void). Maybe we want to return this in both cases, or make Imports return IScriptPackSettingsReferences to allow some flexibility

I also got this working with REPl - how do we proceed? PR against your fork?
2014-02-20_0934

@filipw
Copy link
Member

filipw commented Feb 20, 2014

or, perhaps, since this is a big feature, how about we have a feature branch on the main repo and we work against that until it's mature enough to be merged?

@glennblock
Copy link
Contributor Author

I like the feature branch idea.

On Thu, Feb 20, 2014 at 5:34 AM, Filip W notifications@github.com wrote:

or, perhaps, since this is a big feature, how about we have a feature
branch on the main repo and we work against that until it's mature enough
to be merged?

Reply to this email directly or view it on GitHubhttps://github.com//issues/238#issuecomment-35621490
.

@filipw
Copy link
Member

filipw commented Feb 24, 2014

great - so could you please push from your repo to a new feature branch in the main repo?

@glennblock
Copy link
Contributor Author

Thinking about this more, I prefer not dealing with too many moving parts
at once. I would like to get what is there settled first and then we can
look at getting in more prs.

On Sunday, February 23, 2014, Filip W notifications@github.com wrote:

great - so could you please push from your repo to a new feature branch in
the main repo?

Reply to this email directly or view it on GitHubhttps://github.com//issues/238#issuecomment-35863155
.

@filipw
Copy link
Member

filipw commented Feb 24, 2014

that was my point though too

the core is obviously done but given that (imho) there is still quite a few
things that ought to happen - like the changes I mentioned in a previous
comment, getting REPL to work, rewriting tests, and perhaps optimizing
performance and lookups I think it would make sense if more people chipped
in to get all those settled, rather than it were all on you.

that's why I proposed PRs against your branch or a feature branch (more
git-flow) to get some load off you
but of course if you prefer to do it differently, it's fine too :)

On 24 February 2014 09:39, Glenn Block notifications@github.com wrote:

Thinking about this more, I prefer not dealing with too many moving parts
at once. I would like to get what is there settled first and then we can
look at getting in more prs.

On Sunday, February 23, 2014, Filip W notifications@github.com wrote:

great - so could you please push from your repo to a new feature branch
in
the main repo?

Reply to this email directly or view it on GitHub<
https://github.com/scriptcs/scriptcs/issues/238#issuecomment-35863155>

.

Reply to this email directly or view it on GitHubhttps://github.com//issues/238#issuecomment-35866489
.

@glennblock
Copy link
Contributor Author

I don't think the core is done, there is more
cleanup at least that was feeling when I committed. :-)

I have no issue with addl PRs, but I would like to first get this cleaned
up in its current state and probably more unit tests.

We have waited a long time for this, can't we take it slow?

On Monday, February 24, 2014, Filip W notifications@github.com wrote:

that was my point though too

the core is obviously done but given that (imho) there is still quite a few
things that ought to happen - like the changes I mentioned in a previous
comment, getting REPL to work, rewriting tests, and perhaps optimizing
performance and lookups I think it would make sense if more people chipped
in to get all those settled, rather than it were all on you.

that's why I proposed PRs against your branch or a feature branch (more
git-flow) to get some load off you
but of course if you prefer to do it differently, it's fine too :)

On 24 February 2014 09:39, Glenn Block <notifications@github.comjavascript:_e(%7B%7D,'cvml','notifications@github.com');>
wrote:

Thinking about this more, I prefer not dealing with too many moving parts
at once. I would like to get what is there settled first and then we can
look at getting in more prs.

On Sunday, February 23, 2014, Filip W <notifications@github.comjavascript:_e(%7B%7D,'cvml','notifications@github.com');>
wrote:

great - so could you please push from your repo to a new feature branch
in
the main repo?

Reply to this email directly or view it on GitHub<
https://github.com/scriptcs/scriptcs/issues/238#issuecomment-35863155>

.

Reply to this email directly or view it on GitHub<
https://github.com/scriptcs/scriptcs/issues/238#issuecomment-35866489>
.

Reply to this email directly or view it on GitHubhttps://github.com//issues/238#issuecomment-35868150
.

@glennblock
Copy link
Contributor Author

I created the scripted-packs branch.

I will be managing the PRs to this branch, so please no one else merge.

Thanks!
Glenn

On Mon, Feb 24, 2014 at 7:34 AM, Glenn Block glenn.block@gmail.com wrote:

I don't think the core is done, there is more
cleanup at least that was feeling when I committed. :-)

I have no issue with addl PRs, but I would like to first get this cleaned
up in its current state and probably more unit tests.

We have waited a long time for this, can't we take it slow?

On Monday, February 24, 2014, Filip W notifications@github.com wrote:

that was my point though too

the core is obviously done but given that (imho) there is still quite a
few
things that ought to happen - like the changes I mentioned in a previous
comment, getting REPL to work, rewriting tests, and perhaps optimizing
performance and lookups I think it would make sense if more people chipped
in to get all those settled, rather than it were all on you.

that's why I proposed PRs against your branch or a feature branch (more
git-flow) to get some load off you
but of course if you prefer to do it differently, it's fine too :)

On 24 February 2014 09:39, Glenn Block notifications@github.com wrote:

Thinking about this more, I prefer not dealing with too many moving
parts
at once. I would like to get what is there settled first and then we can
look at getting in more prs.

On Sunday, February 23, 2014, Filip W notifications@github.com wrote:

great - so could you please push from your repo to a new feature
branch
in
the main repo?

Reply to this email directly or view it on GitHub<
https://github.com/scriptcs/scriptcs/issues/238#issuecomment-35863155>

.

Reply to this email directly or view it on GitHub<
https://github.com/scriptcs/scriptcs/issues/238#issuecomment-35866489>
.

Reply to this email directly or view it on GitHubhttps://github.com//issues/238#issuecomment-35868150
.

@adamralph
Copy link
Contributor

I feel this should be more than P3 😉.

It's a major win for the project.

@glennblock
Copy link
Contributor Author

That's not what defines priority. It's not a blocker or something people
must have. It's something nice true...

On Tue, Jun 17, 2014 at 11:56 PM, Adam Ralph notifications@github.com
wrote:

I feel this should be more than P3 [image: 😉].

It's a major win for the project.


Reply to this email directly or view it on GitHub
#238 (comment).

@BrendanThompson
Copy link

@glennblock it's blocking awesomeness! :p will be really awesome when this one comes through. Definitely a handy feature of node being able to call modules in like that!

@glennblock
Copy link
Contributor Author

This is still in progress. I have rethought this and am taking a different route now which will be more efficient, remove any extra load-time impact and be cleaner. I hope to make progress in the next month or so.

@adamralph
Copy link
Contributor

Dropped in favour of #909

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

No branches or pull requests

6 participants