-
Notifications
You must be signed in to change notification settings - Fork 552
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
Comments
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" I am not able to reproduce |
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. It might be an unintended consequence of a <script> in a component. dotnet/aspnetcore#16218 |
I think than scripts cannot be dynamic as styles. And adding and removing them should be done with full page refresh. |
@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 ). |
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. |
OK, so I created an External module to test this and it worked fine: Server\wwwroot\Module.js:
Client\Interop.cs:
Client\Index.razor
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. |
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.
I added logging to OnAfterRenderAsync around the interop calls, and the messages were logged too.
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? 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. |
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 |
Thanks. I will test it this evening. |
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) |
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. |
Thank you for verifying. I think I will reach out to Steve Sanderson regarding the recommended way to solve this probiem. |
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. |
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? |
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. 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. |
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? |
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: [ |
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. |
@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:
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. |
@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. 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. 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: 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? |
“ 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. |
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. In the ThemeBuilder, I added the Quill scripts so they are loaded before the editor is opened.
The result was fewer network calls, and also no issues opening the editor. |
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. |
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? |
The performance gains might have been sacrificed when
I believe that includeScript() is currently replacing the script tags which are loading and unloading. #516 should address that problem. |
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. |
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 ). |
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. |
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? |
And yes, I also noticed that the current includeScript() method that we have in Oqtane has better support for crossorigin and integrity than LoadJS |
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. |
I may have been mistaken about integrity and crossorigin support... it appears that LoadJS can handle it just fine:
|
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 😆).
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.
The module can add the interop script and safely wait until it is loaded.
|
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:
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? |
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 Stylesheet grooming is necessary and it should be done in the Resources. It is working very well too ⭐ @sbwalker
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. |
@sbwalker I created a branch with the changes I described above. |
@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! |
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. |
@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 ).
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 ).
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. |
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. |
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. |
@sbwalker I created the PR with the modifications that I have made. Please add your additions and we can finally close this one 🎉 |
…les and themes
refactoring of #518 to simplify registration of scripts in modules and themes
@jimspillane can you do me a favor and verify the following behavior:
In the browser dev tools console I see: and when I inspect the page source I see:
( it appears to be loading the interop script twice ) |
LoadJS is blowing up on the bundle name(Quill) that is already defined kubetail-org/loadjs#72 I will patch createQuill to test The multiple interop scripts answers #3 in the comment above #518 (comment)
If there is no bundle, LoadJS will reload the script. Creating a bundle and using
|
I got Quill working by refactoring the createQuill method:
|
see changes in #621 - everything seems to be working |
Looks great Shaun, thanks! I will test it out later tonight. One thing we might want to consider is setting |
@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. |
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.
I tried replacing it with the following but it did not work
Do you know of a work-around? |
What are the errors you are seeing? |
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.
I thought replacing the PackageReference with the latest would work, but it threw even more errors. |
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? |
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 |
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.
The text was updated successfully, but these errors were encountered: