Add support for configuration #19544

pranavkm · 2020-03-03T22:14:29Z

SteveSandersonMS · 2020-03-03T22:24:38Z

This looks like it will be great. I’ve only skim read it so far and will review in more detail tomorrow.

One thing that would be really interesting info would be the impact on linked app size from having all the config logic versus none of it (stripping out any existing uses of those APIs). That would give us a basis for whether or not to do fancier things to increase linkability. Currently this implementation means we’ll always keep in the config parsers etc whether or not they will be used.

src/Components/Web.JS/src/Boot.WebAssembly.ts

javiercn

Just skimmed through it, but it looks to a great start!

src/Components/Web.JS/src/Boot.WebAssembly.ts

src/Components/WebAssembly/WebAssembly/src/Hosting/WebAssemblyHostBuilder.cs

src/Components/Web.JS/src/Boot.WebAssembly.ts

SteveSandersonMS · 2020-03-04T15:43:47Z

src/Components/Web.JS/src/Platform/Mono/MonoPlatform.ts

+  toDotNetString: (string: String): System_String => {
+    const binding = window['BINDING'];
+    return binding.js_string_to_mono_string(string);
+  },


While you're adding this kind of thing, would you mind checking if there's a similar (and better) implementation we could put into toJavaScriptString in this file?

The current implementation contains a weird comment we inherited from the Mono sources:

// FIXME this is wastefull, we could remove the temp malloc by going the UTF16 route // FIXME this is unsafe, cuz raw objects could be GC'd.

If there's a "non-wastefull" version of this logic in BINDING, we should consider using it.

Also, rather than using window['BINDING'] in a loosely-typed way, would you consider declaring BINDING inside MonoTypes.d.ts so those functions can just be called using normal strongly-typed syntax? Doesn't need to list all of the BINDING APIs; just the ones we're using. We can add to it over time. Then our wrapper methods like toDotNetString might not even need to exist at all.

src/Components/Web.JS/src/Platform/Platform.ts

src/Components/Web.JS/src/Platform/WebAssemblyResourceLoader.ts

SteveSandersonMS · 2020-03-04T15:51:59Z

src/Components/Web.JS/src/Platform/WebAssemblyResourceLoader.ts

+      return results;
+    }
+
+    const defaultAppSettingsJson = 'appsettings.json';


Just to check, do we definitely want to be case-sensitive like this? What are the chances that someone calls it appSettings.json or AppSettings.json? Is it desirable to make it work in these cases too?

src/Components/Web.JS/src/Platform/WebAssemblyResourceLoader.ts

src/Components/WebAssembly/Build/src/targets/Blazor.MonoRuntime.targets

src/Components/WebAssembly/WebAssembly/src/Hosting/WebAssemblyHostBuilder.cs

src/Components/test/E2ETest/Tests/WebAssemblyConfigurationHostedTest.cs

pranavkm

🆙 📅

src/Components/WebAssembly/Build/src/targets/Blazor.MonoRuntime.targets

src/Components/WebAssembly/WebAssembly/src/Hosting/WebAssemblyHostBuilder.cs

src/Components/test/E2ETest/Tests/WebAssemblyConfigurationHostedTest.cs

pranavkm · 2020-03-04T22:26:25Z

src/Components/Web.JS/src/Platform/Mono/MonoTypes.d.ts

+  function js_string_to_mono_string(jsString: string): Pointer;
+  function js_typed_array_to_array(array: Uint8Array): Pointer;
+  function js_typed_array_to_array<T>(array: Array<T>): Pointer;
+  function conv_string(dotnetString: System_String | null): string | null;


This is the replacement to toJavaScriptString. However the mono code has the exact warnings about being unsafe that our function had. @lewing said he'd investigate their code. That said, we use this function indirectly as part of mono_bind_static_method, so it's not making things worse.

Mono's bindings also has something fairly close to toUint8Array, but it does not guarantee an UInt8Array, so I'm leaving that be for now.

Suggested change

function conv_string(dotnetString: System_String | null): string | null;

function conv_string(dotnetString: System_String): string | null;

... because System_String includes the .NET notion of null. There aren't any cases where we'd need to be passing a JavaScript null here, are there?

pranavkm · 2020-03-04T22:27:29Z

src/Components/Web.JS/src/Platform/Mono/MonoTypes.d.ts

+// We don't actually instantiate any of these at runtime. For perf it's preferable to
+// use the original 'number' instances without any boxing. The definitions are just
+// for compile-time checking, since TypeScript doesn't support nominal types.
+declare interface MethodHandle { MethodHandle__DO_NOT_IMPLEMENT: any }


I'm not entirely sure if this is still equivalent to https://github.com/dotnet/aspnetcore/pull/19544/files#diff-76be3ee9616d4fc0285c84959f51e891L27. Some of these types have to exist here for the typings in Binding to work correctly.

Sorry, I don't understand the question. If there's some practical reason why these definitions need to be moved, let's discuss.

SteveSandersonMS · 2020-03-09T10:00:46Z

src/Components/Web.JS/src/Platform/Mono/MonoTypes.d.ts

+declare namespace BINDING {
+  function js_string_to_mono_string(jsString: string): Pointer;
+  function js_typed_array_to_array(array: Uint8Array): Pointer;
+  function js_typed_array_to_array<T>(array: Array<T>): Pointer;


Suggested change

function js_typed_array_to_array<T>(array: Array<T>): Pointer;

function js_typed_array_to_array<T>(array: Array<T>): System_Array<any>;

SteveSandersonMS · 2020-03-09T10:05:12Z

src/Components/Web.JS/src/Platform/Mono/MonoTypes.d.ts

+declare interface System_Object { System_Object__DO_NOT_IMPLEMENT: any }
+declare interface System_String extends System_Object { System_String__DO_NOT_IMPLEMENT: any }
+declare interface System_Array<T> extends System_Object { System_Array__DO_NOT_IMPLEMENT: any }
+declare interface Pointer { Pointer__DO_NOT_IMPLEMENT: any }


I don't think any of these definitions (System_Object to Pointer) should be moved from Platform.ts into this file.

As long as we're keeping Platform.ts, then the dependencies should be from MonoPlatform.ts to Platform.ts, not the other way around.

src/Components/WebAssembly/WebAssembly/src/Hosting/WebAssemblyHostBuilder.cs

SteveSandersonMS

Looks great!

If we instead do what the extension method does, the linker strips out all of these IFileProvider assemblies. The total download size goes up by 13k.

That seems like pretty good value for 13k. The previous 85kb might have given us reason to change the APIs, but just 13kb seems below the threshold for such a useful feature. (We don't currently have any formal process for making these size tradeoff judgements, but maybe that's something we should consider for the future.) Thanks for investigating this.

The only remaining point I'd raise is that we're still being case-sensitive about the names of the appsettings.* files. Is the same true for server-side ASP.NET Core? We'll probably save everyone some headaches by having the same degree of case (in)sensitivity as the server. Could you comment on this?

If the intention is that the filenames should be accepted case-insensitively, then perhaps the E2E tests could deliberately use nonstandard casing to prove this.

src/Components/Web.JS/src/Platform/Mono/MonoPlatform.ts

Fixes #18675

Co-Authored-By: Javier Calvarro Nelson <[email protected]> Co-Authored-By: Steve Sanderson <[email protected]>

javiercn

This looks great!

mrpmorris · 2020-03-24T23:21:24Z

Will the contents retrieved ultimately be what was embedded into the built binaries?

If so, this would mean a new build per environment? If not, how will a PWA be able to load its config?

SteveSandersonMS · 2020-03-25T13:18:32Z

@mrpmorris No, the point of config is to keep the file editable. For example, you may have devops people who need to take a single app build and deploy it to multiple environments, changing aspects of the config without rebuilding. Maybe they need to change the config on an ad-hoc basis while the app is in production. If we embedded the data in the binary, it would be no more useful than just having a set of consts in your source code.

If not, how will a PWA be able to load its config?

Same as any other static asset. The config JSON file will be cached for offline use. Note that with PWAs specifically, since the configs are cached, you can only edit it when making a new deployment. There's no point editing it between deployments because users will already have a cached version of the file which would continue to be used.

mrpmorris · 2020-03-25T14:39:40Z

I look forward to trying it. Thank you!

pranavkm requested a review from SteveSandersonMS as a code owner March 3, 2020 22:14

ghost added the area-blazor Includes: Blazor, Razor Components label Mar 3, 2020

pranavkm requested a review from javiercn March 3, 2020 22:14

pranavkm added this to the blazor-wasm-3.2-preview3 milestone Mar 3, 2020

lafriks reviewed Mar 4, 2020

View reviewed changes

src/Components/Web.JS/src/Boot.WebAssembly.ts Outdated Show resolved Hide resolved

javiercn reviewed Mar 4, 2020

View reviewed changes

src/Components/Web.JS/src/Boot.WebAssembly.ts Outdated Show resolved Hide resolved

javiercn reviewed Mar 4, 2020

View reviewed changes

src/Components/Web.JS/src/Boot.WebAssembly.ts Outdated Show resolved Hide resolved

javiercn reviewed Mar 4, 2020

View reviewed changes

pranavkm force-pushed the prkrishn/config branch from 4a62124 to f40f3a1 Compare March 4, 2020 15:15

pranavkm requested a review from dougbu as a code owner March 4, 2020 15:15