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

Editor error thrown: Quill is not defined #518

Closed
jimspillane opened this issue May 20, 2020 · 107 comments · Fixed by #608
Closed

Editor error thrown: Quill is not defined #518

jimspillane opened this issue May 20, 2020 · 107 comments · Fixed by #608

Comments

@jimspillane
Copy link
Contributor

An error is thrown when opening the Rich Text Editor for the first time or if reloading the page using Empty Cache and Hard Reload. Maybe the quill-interop.js script is not loaded before the CreateEditor interop call is executed.

image

Exception thrown: 'Microsoft.JSInterop.JSException' in System.Private.CoreLib.dll
Microsoft.AspNetCore.Components.Server.Circuits.RemoteRenderer: Warning: Unhandled exception rendering component: QuillBlotFormatter is not defined
ReferenceError: QuillBlotFormatter is not defined
    at Object.createQuill (http://localhost:44357/js/quill-interop.js:6:49)
    at http://localhost:44357/_framework/blazor.server.js:8:31421
    at new Promise (<anonymous>)
    at e.beginInvokeJSFromDotNet (http://localhost:44357/_framework/blazor.server.js:8:31390)
    at http://localhost:44357/_framework/blazor.server.js:1:19202
    at Array.forEach (<anonymous>)
    at e.invokeClientMethod (http://localhost:44357/_framework/blazor.server.js:1:19173)
    at e.processIncomingData (http://localhost:44357/_framework/blazor.server.js:1:17165)
    at e.connection.onreceive (http://localhost:44357/_framework/blazor.server.js:1:10276)
    at WebSocket.i.onmessage (http://localhost:44357/_framework/blazor.server.js:1:38091)

Microsoft.JSInterop.JSException: QuillBlotFormatter is not defined
ReferenceError: QuillBlotFormatter is not defined
    at Object.createQuill (http://localhost:44357/js/quill-interop.js:6:49)
    at http://localhost:44357/_framework/blazor.server.js:8:31421
    at new Promise (<anonymous>)
    at e.beginInvokeJSFromDotNet (http://localhost:44357/_framework/blazor.server.js:8:31390)
    at http://localhost:44357/_framework/blazor.server.js:1:19202
    at Array.forEach (<anonymous>)
    at e.invokeClientMethod (http://localhost:44357/_framework/blazor.server.js:1:19173)
    at e.processIncomingData (http://localhost:44357/_framework/blazor.server.js:1:17165)
    at e.connection.onreceive (http://localhost:44357/_framework/blazor.server.js:1:10276)
    at WebSocket.i.onmessage (http://localhost:44357/_framework/blazor.server.js:1:38091)
   at Microsoft.JSInterop.JSRuntime.InvokeWithDefaultCancellation[T](String identifier, Object[] args)
   at Oqtane.Modules.Controls.RichTextEditor.OnAfterRenderAsync(Boolean firstRender) in D:\temp\Oqtane.Client\Modules\Controls\RichTextEditor.razor:line 120
   at Microsoft.AspNetCore.Components.RenderTree.Renderer.GetErrorHandledTask(Task taskToHandle)
Microsoft.AspNetCore.Components.Server.Circuits.CircuitHost: Error: Unhandled exception in circuit 'Ie-x6S-hNCalIyPqBsJknPK9X9iz3PXHCuHPLqb_08o'.

Microsoft.JSInterop.JSException: QuillBlotFormatter is not defined
ReferenceError: QuillBlotFormatter is not defined
    at Object.createQuill (http://localhost:44357/js/quill-interop.js:6:49)
    at http://localhost:44357/_framework/blazor.server.js:8:31421
    at new Promise (<anonymous>)
    at e.beginInvokeJSFromDotNet (http://localhost:44357/_framework/blazor.server.js:8:31390)
    at http://localhost:44357/_framework/blazor.server.js:1:19202
    at Array.forEach (<anonymous>)
    at e.invokeClientMethod (http://localhost:44357/_framework/blazor.server.js:1:19173)
    at e.processIncomingData (http://localhost:44357/_framework/blazor.server.js:1:17165)
    at e.connection.onreceive (http://localhost:44357/_framework/blazor.server.js:1:10276)
    at WebSocket.i.onmessage (http://localhost:44357/_framework/blazor.server.js:1:38091)
   at Microsoft.JSInterop.JSRuntime.InvokeWithDefaultCancellation[T](String identifier, Object[] args)
   at Oqtane.Modules.Controls.RichTextEditor.OnAfterRenderAsync(Boolean firstRender) in D:\temp\Oqtane.Client\Modules\Controls\RichTextEditor.razor:line 120
   at Microsoft.AspNetCore.Components.RenderTree.Renderer.GetErrorHandledTask(Task taskToHandle)
Microsoft.AspNetCore.Routing.EndpointMiddleware: Information: Executed endpoint '/_blazor'
Microsoft.AspNetCore.Hosting.Diagnostics: Information: Request finished in 854.0505ms 101 
[2020-05-20T19:11:04.558Z] Error: There was an unhandled exception on the current circuit, so this circuit will be terminated. For more details turn on detailed exceptions by setting 'DetailedErrors: true' in 'appSettings.Development.json' or set 'CircuitOptions.DetailedErrors'. 
[2020-05-20T19:11:04.560Z] Information: Connection disconnected.
@sbwalker
Copy link
Member

sbwalker commented May 21, 2020

if you use your browser tools to inspect the page markup are you able to verify that the quill scripts are registered at the bottom of the page body? They should be named:

"js/quill1.3.6.min.js"
"js/quill-blot-formatter.min.js"
"js/quill-interop.js"

I am not able to reproduce

@jimspillane
Copy link
Contributor Author

Yes, the quill scripts are loading; however, I think the timing of the interop call to createQuill() is ahead of the scripts loading. I changed the network to a slow 3G setting in devtools, and it shows the blazor circuit terminated before the quill scripts loaded.

image

It might be an unintended consequence of a <script> in a component. dotnet/aspnetcore#16218

@PavelVsl
Copy link
Contributor

I think than scripts cannot be dynamic as styles. And adding and removing them should be done with full page refresh.

@sbwalker
Copy link
Member

sbwalker commented May 21, 2020

@jimspillane we are not embedding <script> elements directly in components. Modules statically register their scripts so that the framework is aware of them. The framework dynamically adds them to the DOM using JSInterop in the ThemeBuilder component in the OnParametersSetAsync() method. It does this BEFORE the module components are dynamically added to the panes on the page. Inside of the RichTextEditor.razor component you will see that the Quill scripts are being referenced in the OnAfterRenderAsync() event - which is supposed to be fired after the component is fully rendered and is the recommended event for referencing JavaScript or the DOM.

In the thread you referenced, if you scroll down to Steve Sanderson's comment on Feb 4, 2019 he says "I'd recommend writing a small bit of JS that loads other JS files, and then calling it from your components via the JS interop APIs.". This is exactly the approach that Oqtane is using.

So before we abandon the current approach completely I think we need to understand the order of events better. Perhaps these Javascript references need to be added to the Head rather than the Body ( which is a very simply change to make ).

@sbwalker
Copy link
Member

BTW: I was able to reproduce this problem using Ctrl-F5 ( clear cache and reload ) and you are correct that it only occurs the first time it tries to render the component - on subsequent renderings of the page it works fine. I have tried moving the JS files to the head but it did not resolve it ( even though I can see in the network tab that the files were downloaded earlier ). I think we need to try and isolate if this is only a problem with Quill - or if it is a general JavaScript loading issue. I am going to experiment a bit more today.

@sbwalker
Copy link
Member

sbwalker commented May 21, 2020

OK, so I created an External module to test this and it worked fine:

Server\wwwroot\Module.js:

window.interop = {
    showAlert: function (message) {
        alert(message);
    },
};

Client\Interop.cs:

using Microsoft.JSInterop;
using System.Threading.Tasks;

namespace Siliqon.Cars
{
    public class Interop
    {
        private readonly IJSRuntime _jsRuntime;

        public Interop(IJSRuntime jsRuntime)
        {
            _jsRuntime = jsRuntime;
        }

        public Task ShowAlert(string message)
        {
            try
            {
                _jsRuntime.InvokeAsync<string>(
                    "interop.showAlert",
                    message);
                return Task.CompletedTask;
            }
            catch
            {
                return Task.CompletedTask;
            }
        }
    }
}

Client\Index.razor

@inject IJSRuntime JsRuntime
...
    public override List<Resource> Resources => new List<Resource>()
    {
        new Resource { ResourceType = ResourceType.Stylesheet, Url = ModulePath() + "Module.css" },
        new Resource { ResourceType = ResourceType.Script, Url = ModulePath() + "Module.js" }
    };
...
    protected override async Task OnAfterRenderAsync(bool firstRender)
    {
        if (firstRender)
        {
            var interop = new Interop(JsRuntime);
            await interop.ShowAlert("testing");
        }
    }

This works fine on first render. So I am thinking that this issue may be related to the RichTextEditor itself. In looking at the code I see that it is doing JSInterop in a completely different way than the core. I will have to explore more.

@jimspillane
Copy link
Contributor Author

Thanks for looking into this @sbwalker.

I created the Cars module above. It worked as expected until I switched the DevTools setting on the Network tab to Slow 3G, then I no longer received the alert.

I added a line to the showAlert() function, and the console message did not show as well.

window.interop = {
    showAlert: function (message) {

        console.log("*** made it to showAlert ***");
        alert(message);
        
    },
};

I added logging to OnAfterRenderAsync around the interop calls, and the messages were logged too.

protected override async Task OnAfterRenderAsync(bool firstRender)
    {
        if (firstRender)
        {
            try
            {
                await logger.LogInformation("************** Before interop");
                var interop = new Interop(JsRuntime);
                await interop.ShowAlert("testing");
                await logger.LogInformation("************** After interop");
            }
            catch (Exception ex)
            {
                await logger.LogError(ex, "Error Loading Car {Error}", ex.Message);
                AddModuleMessage("Error Loading Car", MessageType.Error);
            }
        }
    }

Just when I thought Blazor was going to solve all the JavaScript problems of the world :) Here are a few questions that I am thinking out loud...

Regarding Quill, do the scripts have dependencies that require them to be loaded in order?
"js/quill1.3.6.min.js"
"js/quill-blot-formatter.min.js"
"js/quill-interop.js"

Since we are injecting the scripts into the DOM, are they being loaded and executed when they are downloaded? I would say yes in a standard page refresh, but since this is in a SPA, does that change behavior of the loading?

Does the OnAfterRenderAsync() event act as a "$(document).ready" event? From what I see, it does not, and the interop calls might fail, even silently. So if we load scripts dynamically, we will need to know when they are ready to be called by the interop. Perhaps a JavaScript to .NET call to set an initialized script state.

@sbwalker
Copy link
Member

sbwalker commented May 21, 2020

I made some changes to the JSInterop for the RichTextEditor and I can no longer reproduce the issue. I am going to merge the changes so that you can do some testing as well ( if you have time ). #522

@jimspillane
Copy link
Contributor Author

Thanks. I will test it this evening.

@jimspillane
Copy link
Contributor Author

I simulated a 3G connection and it threw an exception on the first load.

scripterror

@sbwalker
Copy link
Member

sbwalker commented May 22, 2020

When you originally logged this issue it was because you were able to reliably create it on a standard first load scenario - correct? The simulated 3G connection was a different (possibly related) aspect you reported later. Are you able to reproduce the problem under the original scenario? I am no longer able to reproduce the issue on a standard first load scenario after the recent code changes (which would mean that there was an improvement in behavior)

@jimspillane
Copy link
Contributor Author

It took four tries(emptied cache) before it failed when running in VS Debug. I switched to Release mode and Start Without Debugging and it fails every time.

@sbwalker
Copy link
Member

sbwalker commented May 22, 2020

Thank you for verifying. I think I will reach out to Steve Sanderson regarding the recommended way to solve this probiem.

@sbwalker
Copy link
Member

I am curious if this is a problem when running on both Blazor Server and Blazor WebAssembly - or just Blazor Server. The error message is talking about circuits which is directly related to SignalR.

@jimspillane
Copy link
Contributor Author

jimspillane commented May 22, 2020

I tested in WebAssembly too, the error message is logged in the console. Regarding the circuit breaking, we might be able to mitigate it with a try/catch.

image

@sbwalker
Copy link
Member

Hmm.... maybe its best to go back to the simplest example - the alert() function, as Quill is a more complex example. You said you were able to reproduce the issue using your external module which is calling the alert() function correct?

@jimspillane
Copy link
Contributor Author

Yes, but only with the Slow 3G.

The other thing I tested was to add a long delay before the call to interop.CreateEditor(). This ensured that the Quill scripts were loaded.

image

If the interop call is fired before the scripts are loaded, then an error is thrown. Perhaps the includeScript() function can also manage an array that contains scripts that have been loaded. Then loaded script array can be checked before executing the interop functions.

@sbwalker
Copy link
Member

That is worth a try. Perhaps we can include another JSInterop method for ensureScriptsLoaded() and call it in ThemeBuilder after the scripts have been added?

@sbwalker
Copy link
Member

Interesting post:

Scripts that are dynamically created and added to the document are async by default, they don’t block rendering and execute as soon as they download, meaning they could come out in the wrong order. However, we can explicitly mark them as not async:

[
'//other-domain.com/1.js',
'2.js'
].forEach(function(src) {
var script = document.createElement('script');
script.src = src;
script.async = false;
document.head.appendChild(script);
});

@jimspillane
Copy link
Contributor Author

That is a good find for dependencies.

I was looking at LABjs and it has some of the features that we might find useful for this use case.

@sbwalker
Copy link
Member

@jimspillane I checked in a couple minor changes. Now the only way I have been able to trigger a loading issue with Quill is when running on Blazor Server and specifying No Caching and simulate 3G connection and Ctrl-F5. The error that is thrown is:

[2020-05-22T15:57:29.615Z] Error: There was an unhandled exception on the current circuit, so this circuit will be terminated. For more details turn on detailed exceptions by setting 'DetailedErrors: true' in 'appSettings.Development.json' or set 'CircuitOptions.DetailedErrors'.

The detailed error is:

Error: Microsoft.JSInterop.JSException: Could not find 'getQuillHTML' in 'window.interop'.

In regards to Quill there are dependencies between the various JavaScript files - which means they need to be loaded in order:

  1. quill1.3.6.min.js
  2. quill-blot-formatter.min.js
  3. quill-interop.js

Somehow when running on Blazor Server and 3G speed, the order seems to be off. And this is after I made the change above to include script.async = false; This seems like a very obscure issue.

@jimspillane
Copy link
Contributor Author

@sbwalker It seems like a delay in the download and execution of the scripts can cause the issue. interop.includeScript() does not "await" for the scripts to be loaded. It appends the script tag and moves on. interop.createQuill() can then throw an exception because the Quill or QuillBlotFormatter objects are not loaded yet. CreateEditor() and LoadEditorContent() both return Task.CompletedTask, so they do not throw an error, but GetHtml() expects a value returned and does throw when the script is not loaded. This is what is happening in the error above.

Overall, the changes perform better, however switching out the Quill script with CDN version causes issues to occur more often. It most likely has to do with the additional delay from not being on the local machine.
new Resource { ResourceType = ResourceType.Script, Url = "https://cdnjs.cloudflare.com/ajax/libs/quill/1.3.6/quill.min.js" },

I had some success using RequireJS. This allows for loading the scripts when the app starts which helps to avoid the timing issues. While the addition of this script goes against the Oqtane philosophy, maybe we can borrow some of the ideas. For example, I am adding a script in the ThemeBuilder.razor page that contains the RequireJS module definition that adds the Quill scripts:
await interop.IncludeScript("LoadQuillModule", "/js/lib/modules/QuillInitLoader.js", "", "body", "", "");

Could the modules expose the script resources they use in a way that when the application starts we can loop through all the scripts and insert them once?

@sbwalker
Copy link
Member

“ Could the modules expose the script resources they use in a way that when the application starts we can loop through all the scripts and insert them once?” - it is already working this way... modules and themes notify the framework about their resources by overriding their Resources property. The router uses this information to determine the resources which are needed on a page and the ThemeBuilder injects all the style sheets and scripts at once.

@jimspillane
Copy link
Contributor Author

Can this be done at the start of the application for all modules? For example, the Quill scripts are re-loaded every time the editor is opened. If they can be loaded once at the startup, we can potentially avoid some of the loading issues and reduce network chatter. The image below contains the network calls when I open and close the editor a few times.

image

In the ThemeBuilder, I added the Quill scripts so they are loaded before the editor is opened.

protected override async Task OnParametersSetAsync()
    {
        var interop = new Interop(JsRuntime);

        await interop.IncludeScript("LoadQuillModule", "/js/lib/modules/QuillInitLoader.js", "", "body", "", "");

The result was fewer network calls, and also no issues opening the editor.

image

@sbwalker
Copy link
Member

It would be possible to do what you are suggesting however there is a rationale behind the current implementation approach. Oqtane assumes that it is more efficient to only load the resources which are needed for the current user for the current page than it is to load all resources referenced by any module/theme that may be installed in the application. For example, if a user is not an Administrator then they cannot edit content. Therefore it would not make sense to force all users to load the Quill style sheets and scripts. Similarly you may have a number of themes installed in your application but you may only be using 1 theme in your site. So it does not make sense to load all of the stylesheets for all of the installed themes if they are not being used. The primary focus in Oqtane is performance so optimizing the loading of resources is important.

@sbwalker
Copy link
Member

I am confused why the Quill scripts are being loaded every time in your browser - and why the browser is not utilizing its native caching capabilities. Do you have caching explicitly disabled in your browser settings?

@jimspillane
Copy link
Contributor Author

The performance gains might have been sacrificed when script.async = false; was added to includeScript().

Do you have caching explicitly disabled in your browser settings?

I believe that includeScript() is currently replacing the script tags which are loading and unloading. #516 should address that problem.

@sbwalker
Copy link
Member

Each page in a site will likely have a different set of resources - it depends on the themes/modules being used on a page. So the list of resources will be different per page and the order of the list will be different. For example on page1 ModuleA's "module.js" might be element1 of the list resulting in an ID of "app-script01" and on page2 it might be element2 in the list resulting in an ID of "app-script02". Since the includeScript() function is based on ID, will the changes you made actually make any difference? It appears that it will only make a difference if the Src for a specific ID remains constant - which is not the case currently.

@sbwalker
Copy link
Member

The script.async = false should not have affected browser caching - it should only affect how scripts are loaded and make the behavior consistent with how they would behave if the <script tags were statically embedded in the page ( which are always synchronously loaded unless there is an explicit async attribute added ).

@jimspillane
Copy link
Contributor Author

Running in Debug mode in VS might be the cause of the files not being cached.

You bring up a good point that a variable ID might be different from page to page. It only works well when the pages are similar. I looked at the output for RequireJS, and it is using a data- attribute to store the name of the required modules. A fixed ID might be useful for some of the scripts so we don't have to match the src.

image

@sbwalker
Copy link
Member

Sorry... I forgot to mention "foundational" dependencies. These will remain in _host.cshtml. JQuery is a foundational dependency. If you rule out foundational dependencies can you come up with a practical example?

@sbwalker
Copy link
Member

And yes, I also noticed that the current includeScript() method that we have in Oqtane has better support for crossorigin and integrity than LoadJS

@sbwalker
Copy link
Member

And I know you were just coming up with a hypothetical example, but I would say that using DataTables breaks the first rule that I outlined above - developers should use a native Blazor solution for tables with paging/sorting and not a JavaScript solution.

@sbwalker
Copy link
Member

I may have been mistaken about integrity and crossorigin support... it appears that LoadJS can handle it just fine:

loadjs('path/lib.js', 'bundle', {
    before: function(path, el) {
        el.integrity = 'xxxx';
        el.crossOrigin = 'anonymous';
    }
});

@jimspillane
Copy link
Contributor Author

I believe one of the goals for LoadJS was a small footprint, so some features require an indirect approach. It is fine for a single dependency, but it gets more interesting as you add more. That's why I chose to build the script rather than make the calls directly(Rule # 1 Since Oqtane is based on Blazor, the goal should be to use as much native Blazor functionality as possible rather than using JavaScript 😆).

loadjs(['https://code.jquery.com/jquery-3.3.1.slim.min.js','https://cdnjs.cloudflare.com/ajax/libs/popper.js/1.14.7/umd/popper.min.js','https://stackpath.bootstrapcdn.com/bootstrap/4.3.1/js/bootstrap.min.js'], Init, {
	async: false,
	before: function(path, scriptEl) {
		if (path === 'https://code.jquery.com/jquery-3.3.1.slim.min.js') scriptEl.crossOrigin = 'anonymous';
		if (path === 'https://code.jquery.com/jquery-3.3.1.slim.min.js') scriptEl.integrity = 'sha384-q8i/X+965DzO0rT7abK41JStQIAqVgRVzpbzo5smXKp4YfRvH+8abtTE1Pi6jizo';
		if (path === 'https://cdnjs.cloudflare.com/ajax/libs/popper.js/1.14.7/umd/popper.min.js') scriptEl.crossOrigin = 'anonymous';
		if (path === 'https://cdnjs.cloudflare.com/ajax/libs/popper.js/1.14.7/umd/popper.min.js') scriptEl.integrity = 'sha384-UO2eT0CpHqdSJQ6hJty5KVphtPhzWj9WO1clHTMGa3JDZwrnQq4sF86dIHNDz0W1';
		if (path === 'https://stackpath.bootstrapcdn.com/bootstrap/4.3.1/js/bootstrap.min.js') scriptEl.crossOrigin = 'anonymous';
		if (path === 'https://stackpath.bootstrapcdn.com/bootstrap/4.3.1/js/bootstrap.min.js') scriptEl.integrity = 'sha384-JjSmVgyd0p3pXB1rRibZUAYoIIy6OrQ6VrjIEaFf/nJGzIxFDsf4x0xIM+B07jRM';
	}
}); 

Regarding the higher level requirements, if I made a module that had JavaScript dependencies, I would use the method used in this #518 (comment) and not use Resources by calling LoadJS directly in the interop.

  1. Let the JavaScript manage the dependencies when it needs them rather than in separate Blazor components executing asynchronously.

  2. Oqtane will provide a way to load interop scripts dynamically. After that, the module is responsible to make additional dependency calls to LoadJS.

  3. JavaScript will be loaded when it is used by the component. This will typically be done by a module calling the interop.

  4. By providing a way for Oqtane to load an interop script, the module can then make the direct calls to LoadJS to manage dependencies.

loadOqtaneInterop: async function(file) {
        const promise = new Promise((resolve, reject) => {
            loadjs(file, { returnPromise: true })
                .then(function () { resolve(true) })
                .catch(function (pathsNotFound) { reject(false) });
        });

        //Wait for promise
        const result = await promise;
        return result;
    }

The module can add the interop script and safely wait until it is loaded.

await oqtaneInterop.LoadOqtaneInterop("js/quill-interop.js");

  1. By offloading the Resources script functionality to the modules interop JavaScript and LoadJS, complex dependencies can be achieved. Sometimes I feel like Blazor is adding to the JavaScript that I need to write rather than reduce it.

@sbwalker
Copy link
Member

sbwalker commented Jun 10, 2020

I like the direction you are going with this... after some experimenting of my own I was coming to the same conclusions. In order for this approach to work, it still hinges on one critical item:

"2. Oqtane will provide a way to load interop scripts dynamically."

Based on the async model in Blazor I understand why you are suggesting to load the interop script from within the component which depends on the script. However I am still trying to understand how to handle the following scenario:

  1. User browses to Page1 which contains ComponentA. ComponentA uses JavaScript and calls the framework method to include the interop script ie. IncludeScript("componentA/interop.js);. This framework method creates a <script> tag in the DOM for ComponentA.
  2. User browses to Page2 which does not contain ComponentA. However the <script> tag for ComponentA is still in the DOM ( it will be there forever in a SPA application unless it is explicitly removed ). Page2 has ComponentB and it uses JavaScript so it adds its own <script> tag to the DOM. As a user navigates around a site should they expect the list of <script> tags to accumulate in the DOM indefinitely?
  3. User browses back to Page1 and ComponentA needs to be rendered again. It has no way of knowing if its <script> tag is already in the DOM or not, so it needs to do a lookup. Does it do the lookup based on an ID? Based on the content of the <script>?

The main reason why the centralized management of Resources exists currently is so that the framework can manage the "clean-up" of the DOM on a page by page basis. The "cleanup" does not deallocate the memory related to the script, it simply grooms the DOM. Maybe this is not as important for scripts as it is for stylesheets?

@jimspillane
Copy link
Contributor Author

As a user navigates around a site should they expect the list of <script> tags to accumulate in the DOM indefinitely?

Yes. From what I understand, removing the <script> element does not unload the script. For example, in the case of quill-interop.js, we should also set Oqtane.RichTextEditor = {}; to remove it from the global namespace. This would be a monumental task for every script that could be dynamically loaded in Oqtane, which is probably why there are few attempts at solving the SPA problem. The best one I saw was Realms API, but at the glacial pace they approve standards, I will be retired before it is available.

Stylesheet grooming is necessary and it should be done in the Resources. It is working very well too ⭐ @sbwalker

  1. User browses back to Page1 and ComponentA needs to be rendered again. It has no way of knowing if its <script> tag is already in the DOM or not, so it needs to do a lookup. Does it do the lookup based on an ID? Based on the content of the <script>?

Good question. There is a bundleIdCache saved that might stop the script reload. But I also noticed that scripts that are loaded without a bundled are not being re-loaded too.

@jimspillane
Copy link
Contributor Author

@sbwalker I created a branch with the changes I described above.
master...jimspillane:AddJavaScriptDependencyManager

It works with Slow 3G!
Slow3G

@sbwalker
Copy link
Member

@jimspillane I had a very busy day at work today so did not get any time to explore this... but I am very excited to see it working!

@jimspillane
Copy link
Contributor Author

No problem. Take a look at it when you have a chance to see if it hits the requirements. I can submit it as a PR if you think it will work for us.

@sbwalker
Copy link
Member

@jimspillane I tried out your solution and it works perfectly. I also did some additional experimentation based on some ideas I had, and I would like to get your feedback...

Background: Modules and Module Controls ( ie. RichTextEditor, etc... ) inherit from ModuleBase. ModuleBase contains a Resources property.

ModuleBase.cs - I added a default implementation for OnAfterRenderAsync(). This uses the assumption that a module will only declare a single interop script - and the interop script is responsible for pulling in any other dependencies using LoadJS. ( Note that in my code I changed the LoadScript() method to use your LoadJS logic ).

        protected override async Task OnAfterRenderAsync(bool firstRender)
        {
            if (firstRender)
            {
                if (Resources != null && Resources.Exists(item => item.ResourceType == ResourceType.Script))
                {
                    var interop = new Interop(JsRuntime);
                    await interop.LoadScript(Resources.FirstOrDefault(item => item.ResourceType == ResourceType.Script).Url);
                }
            }
        }

RichTextEditor.razor: I declared the interop script resource and I added a call to the base implementation of OnAfterRenderAsync ( only because RichTextEditor needs to do other things in OnAfterRenderAsync - normally this would not be necessary ).

    public override List<Resource> Resources => new List<Resource>()
    {
        new Resource { ResourceType = ResourceType.Script, Url = "js/quill-interop.js" }
    };
   ...
    protected override async Task OnAfterRenderAsync(bool firstRender)
    {
        if (firstRender)
        {
            await base.OnAfterRenderAsync(firstRender);
            var interop = new RichTextEditorInterop(JsRuntime);
            ...
       }
    }

With this change, the solution still works and the scripts are loaded properly. The benefit is that a developer declares their scripts the same way they do currently, and the framework takes care of the complexity of registering them.

Note that Themes and Theme Controls ( ie. Login button ) inherit from ThemeBase... and ThemeBase contains a Resources property - so the exact same logic will work for themes as well.

@sbwalker
Copy link
Member

I suppose it is possible that a module could have more than 1 interop script if it is utilizing more than 1 JavaScript library... so the OnAfterRenderAsync() method may need to support multiple script resources. However if those JavaScript libraries have other dependencies, they would have to load them natively using LoadJS.

@ADefWebserver
Copy link
Member

I suppose it is possible that a module could have more than 1 interop script if it is utilizing more than 1 JavaScript library... so the OnAfterRenderAsync() method may need to support multiple script resources. However if those JavaScript libraries have other dependencies, they would have to load them natively using LoadJS.

I do not think that it is unreasonable for a module developer to put all their .js resources in one script if that means that Oqtane can load the .js scripts optimally.

If you guys make this work, this is a very-big-deal in my opinion.

@jimspillane
Copy link
Contributor Author

That would work for modules that fit that use case. If there are complex dependencies, the developer has the option to use the lower-level functions that are exposed by the Oqtane interop and LoadJS. It is the best of both worlds.

I was focused on getting the Quill scripts working, so I didn't put much thought into the Themes. I think we have a winner if your added logic works well with the themes too.

@jimspillane
Copy link
Contributor Author

@sbwalker I created the PR with the modifications that I have made. Please add your additions and we can finally close this one 🎉

sbwalker added a commit to sbwalker/oqtane.framework that referenced this issue Jun 14, 2020
sbwalker added a commit that referenced this issue Jun 14, 2020
refactoring of #518 to simplify registration of scripts in modules and themes
@sbwalker
Copy link
Member

sbwalker commented Jun 16, 2020

@jimspillane can you do me a favor and verify the following behavior:

  • launch Oqtane and login

  • on the Home page click the pencil to go into Edit Mode

  • click Edit for the "Welcome to Oqtane..." Html/Text module

  • note that the Rich Text Editor loads properly

  • close the Edit Html/Text modal

  • click Edit for the "Welcome to Oqtane..." Html/Text module once again

  • the Rich Text Editor does not load properly this time

  • the gold bar for "An Unhandled Exception Occurred" is displayed

In the browser dev tools console I see:
Error: Microsoft.JSInterop.JSException: can't access property "root", editorElement.__quill is undefined
getQuillHTML@http://localhost:44357/js/quill-interop.js:37:9
beginInvokeJSFromDotNet/r<@http://localhost:44357/_framework/blazor.server.js:8:31421

and when I inspect the page source I see:

<script src="js/quill-interop.js" async=""></script>
<script src="js/quill-interop.js" async=""></script>

( it appears to be loading the interop script twice )

@jimspillane
Copy link
Contributor Author

LoadJS is blowing up on the bundle name(Quill) that is already defined kubetail-org/loadjs#72 I will patch createQuill to test loadjs.isDefined("Quill") before trying to reload the bundle.

The multiple interop scripts answers #3 in the comment above #518 (comment)

  1. User browses back to Page1 and ComponentA needs to be rendered again. It has no way of knowing if its <script> tag is already in the DOM or not, so it needs to do a lookup. Does it do the lookup based on an ID? Based on the content of the <script>?

If there is no bundle, LoadJS will reload the script. Creating a bundle and using loadjs.isDefined() will keep it from creating multiple scripts.

loadScript: async function (path) {
        const promise = new Promise((resolve, reject) => {

            if (loadjs.isDefined(path)) {
                resolve(true);
                return;
            }

            loadjs(path, path,
                    { returnPromise: true, async: false })
                .then(function () { resolve(true) })
                .catch(function (pathsNotFound) { reject(false) });
        });

        const result = await promise;
        return;
    }

@sbwalker
Copy link
Member

I got Quill working by refactoring the createQuill method:

    createQuill: async function (
        quillElement, toolBar, readOnly,
        placeholder, theme, debugLevel) {

        if (!loadjs.isDefined('Quill')) {
            const loadQuill = loadjs(['js/quill1.3.6.min.js', 'js/quill-blot-formatter.min.js'], 'Quill',
                { async: true, returnPromise: true })
                .then(function () {
                    Quill.register('modules/blotFormatter', QuillBlotFormatter.default);
                })
                .catch(function (pathsNotFound) { });
            await loadQuill;
        }

        var options = {
            debug: debugLevel,
            modules: {
                toolbar: toolBar,
                blotFormatter: {}
            },
            placeholder: placeholder,
            readOnly: readOnly,
            theme: theme
        };

        new Quill(quillElement, options);
    },

@sbwalker
Copy link
Member

see changes in #621 - everything seems to be working

@jimspillane
Copy link
Contributor Author

Looks great Shaun, thanks! I will test it out later tonight. One thing we might want to consider is setting async : true to async : false .

@sbwalker
Copy link
Member

@jimspillane see #622 - I refactored script management so that it now based on the collection of scripts rather than registering them individually ( similar to how it worked previously ), allows the developer to specify a Bundle property, handles integrity and crossorigin, and switched from sync to async ( as per your suggestion above ). The benefit is that Oqtane does not need to make multiple JSInterop calls for each script and in most cases a developer will not need to write any loadjs logic in their interop script - the framework takes care of script registration.

@jimspillane
Copy link
Contributor Author

Looks good @sbwalker

I tried testing it in an external module and I am getting issues involving the PackageReference to Oqtane when I try to add a Bundle to the Resource.

  <ItemGroup>
    <PackageReference Include="Oqtane.Server" Version="1.0.0" />
    <PackageReference Include="Oqtane.Shared" Version="1.0.0" />
  </ItemGroup>

I tried replacing it with the following but it did not work

  <ItemGroup>
    <Reference Include="Oqtane.Server">
      <HintPath>..\..\oqtane.framework\Oqtane.Server\bin\Debug\netcoreapp3.1\Oqtane.Server.dll</HintPath>
    </Reference>
    <Reference Include="Oqtane.Shared">
      <HintPath>..\..\oqtane.framework\Oqtane.Server\bin\Debug\netcoreapp3.1\Oqtane.Shared.dll</HintPath>
    </Reference>
  </ItemGroup>

Do you know of a work-around?

@sbwalker
Copy link
Member

What are the errors you are seeing?

@jimspillane
Copy link
Contributor Author

Adding Bundle to a new external module throws an error. It looks like the reference is not using the latest code merged but the nuget package.

    public override List<Resource> Resources => new List<Resource>()
    {
        new Resource { ResourceType = ResourceType.Stylesheet, Url = ModulePath() + "Module.css" },
        new Resource { ResourceType = ResourceType.Script, Bundle = "Quill", Url = "js/quill1.3.6.min.js" },
        new Resource { ResourceType = ResourceType.Script, Bundle = "Quill", Url = "js/quill-blot-formatter.min.js" },
        new Resource { ResourceType = ResourceType.Script, Bundle = "Quill", Url = "js/quill-interop.js" }
    };

I thought replacing the PackageReference with the latest would work, but it threw even more errors.

@sbwalker
Copy link
Member

sbwalker commented Jun 17, 2020

If you used the external module template in the Module Creator to create your module then the project files contain references to the 1.0.0 Nuget packages - which are based on the official 1.0.0 release bits and do not contain any of the changes in the past month ( so Bundle does not exist ). You should be able to replace the package reference with an assembly reference to the latest assemblies you built from source - what are the errors you are seeing when you do this?

@jimspillane
Copy link
Contributor Author

I needed to change all three of the projects to the local references and it compiled.

The changes work for both internal and external modules. I also checked using multiple modules on the same page. The only issue is the async in #624

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

Successfully merging a pull request may close this issue.

9 participants