-
Notifications
You must be signed in to change notification settings - Fork 385
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
extract api to configure aspnet hosting to expose .NET Interactive api #802
Conversation
c3082a5
to
da6a981
Compare
3ab0cb6
to
5ba56e8
Compare
3aa6fbc
to
4c5fae0
Compare
61bed59
to
e9210dc
Compare
{ | ||
public static class JavascriptUtilities | ||
{ | ||
public static string EnsureRequireJs(Uri requireJsUri = null, string callBackName = null) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This name and return type could be clearer. Ensure*
usually means ensure some thing has been done, and signals idempotency. This returns a string
.
What's it really doing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that returns the javascript code snippet that checks and pull in if needed the requirejs library
{ | ||
public class FileProvider : IFileProvider, IDisposable | ||
{ | ||
private readonly EmbeddedFileProvider _root; | ||
private readonly IDisposable _eventSubscription; | ||
private readonly ConcurrentDictionary<string, EmbeddedFileProvider> _providers = new ConcurrentDictionary<string, EmbeddedFileProvider>(); | ||
|
||
public FileProvider(Kernel kernel) | ||
public FileProvider(Kernel kernel, Assembly rootProvider) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
public FileProvider(Kernel kernel, Assembly rootProvider) | |
public FileProvider(Kernel kernel, Assembly rootProviderAssembly) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sounds good
}); | ||
return builder; | ||
} | ||
public static IServiceCollection AddDotnetInteractive(this IServiceCollection services) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe the naming should also include the fact that this is adding HTTP-specific functionality?
|
||
return services; | ||
} | ||
public static IApplicationBuilder UseDotNetInteractive<T>(this IApplicationBuilder app, T kernel, Assembly staticResourceRoot, HttpProbingSettings httpProbingSettings, HttpPort httpPort) where T : Kernel |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto.
@@ -305,14 +300,8 @@ Command HttpServer() | |||
httpCommand.Handler = CommandHandler.Create<StartupOptions, KernelHttpOptions, IConsole, InvocationContext>( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just noticed that KernelHttpOptions
has nothing to do with HTTP. Does it need to exist and why would it be different from the already-existing DefaultKernel
concept?
2887f15
to
11fb909
Compare
docs/javascript-overview.md
Outdated
|
||
Here is an example. | ||
![image](https://user-images.githubusercontent.com/375556/95768858-e6be3500-0cae-11eb-925c-53d46670bd09.png) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's use text rather than images to document APIs. This will be hard to search for otherwise.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed image and made test example that users can actually use and try
docs/javascript-overview.md
Outdated
Here is an example. | ||
![image](https://user-images.githubusercontent.com/375556/95768858-e6be3500-0cae-11eb-925c-53d46670bd09.png) | ||
The `interactive.configureRequire` takes as input a [requirejs configuration object](https://www.tutorialspoint.com/requirejs/requirejs_configuration.htm) and returns a function that you can now use to load the dependecies. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's add a simple, working code sample.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
@@ -529,7 +515,7 @@ static HttpPortRange ParsePortRangeOption(ArgumentResult result) | |||
SetUpFormatters(frontendEnvironment, startupOptions, TimeSpan.FromSeconds(15)); | |||
|
|||
kernel.DefaultKernelName = defaultKernelName; | |||
|
|||
services.AddKernel(kernel); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doing this here seems to blur some boundaries. I would expect the caller to this method to handle registration, and for this method to be non-side-affecting.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that part of code has always been like that as the kernel is created as side effect while executing the command line parser. the caller is passing in the ServiceCollection and merging it with the one used during asp.net configuration
docs/javascript-overview.md
Outdated
}); | ||
``` | ||
|
||
Now you can require the module and use it. Using `#!html` to inject an `svg` element and then `#!js` to load the `d3.js` library and append a circle to the dom |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now you can require the module and use it. Using `#!html` to inject an `svg` element and then `#!js` to load the `d3.js` library and append a circle to the dom | |
Now you can require the module and use it. You can use the `#!html` magic command to inject an SVG element and then the `#!js` magic command to load the D3.js library and append a circle. |
|
||
#!js | ||
#!html | ||
<svg id="renderTarget" width=300 height=300></svg> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You have an extra #!js
here and the renderTarget
is being added twice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
docs/javascript-overview.md
Outdated
|
||
``` | ||
#!html | ||
<sgv id"renderTarget"></svg> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<sgv id"renderTarget"></svg> | |
<svg id"renderTarget"></svg> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
remove unused js function and rename helper to condifugreRequire for general require and configureRequireFromExtension for the require function taht will load static content form interactive extensions
Co-authored-by: Jon Sequeira <[email protected]>
Co-authored-by: Jon Sequeira <[email protected]>
Co-authored-by: Jon Sequeira <[email protected]>
No description provided.