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

Runtime and IPC only injected on index.html file #2047

Closed
mholt opened this issue Nov 3, 2022 · 18 comments · Fixed by #2203
Closed

Runtime and IPC only injected on index.html file #2047

mholt opened this issue Nov 3, 2022 · 18 comments · Fixed by #2203
Assignees
Labels
Bug Something isn't working

Comments

@mholt
Copy link
Contributor

mholt commented Nov 3, 2022

Description

My frontend is vanilla JS, so I have multiple HTML files. It's a good-ol', boring website from the early 2000s. Relatively little client-side JS (no routing, etc).

I noticed that runtime.js and ipc.js are only injected for index.html:

func (d *AssetServer) processIndexHTML(indexHTML []byte) ([]byte, error) {
htmlNode, err := getHTMLNode(indexHTML)
if err != nil {
return nil, err
}
if d.appendSpinnerToBody {
err = appendSpinnerToBody(htmlNode)
if err != nil {
return nil, err
}
}
if err := insertScriptInHead(htmlNode, runtimeJSPath); err != nil {
return nil, err
}
if err := insertScriptInHead(htmlNode, ipcJSPath); err != nil {
return nil, err
}

This came after some head-scratching as to why Uncaught TypeError: window.go is undefined errors kept appearing but it worked fine on the index page.

Additionally, hot reloading doesn't seem to work on any non-index file, but maybe that's a separate issue.

To Reproduce

  1. Make a file that is not index.html
  2. Navigate to that file in the front-end
  3. The console shows errors because the runtime and IPC are not available

Expected behaviour

I think it would make more sense to either inject these on all HTML pages, or to not inject them at all.

Screenshots

No response

Attempted Fixes

Fortunately, I can work around this by manually adding them:

<script src="/wails/ipc.js"></script>
<script src="/wails/runtime.js"></script>

Proposed solutions:

  1. Inject those on all HTML pages
  2. Don't inject them on any pages (make the user do it in their HTML)
  3. Since the user already has to import App.js to access bound methods, maybe just include/import those files as part of that, so it can happen implicitly?

System Details

Wails CLI v2.1.0

Scanning system - Please wait (this may take a long time)...Done.

System
------
OS:		Pop!_OS
Version: 	22.04
ID:		pop
Go Version:	go1.19.1
Platform:	linux
Architecture:	amd64

Wails
------
Version: 		v2.1.0
Revision:		e545a7747f97ffc1f130b61d0b2e3f5de3b23279
Modified:		false
Package Manager: 	apt

Dependency	Package Name		Status		Version
----------	------------		------		-------
*docker 	docker.io 		Available 	20.10.12-0ubuntu4
gcc 		build-essential 	Installed 	11.3.0
libgtk-3 	libgtk-3-dev 		Installed 	3.24.33-1ubuntu2
libwebkit 	libwebkit2gtk-4.0-dev 	Installed 	2.36.8-0ubuntu0.22.04.1
npm 		npm 			Installed 	8.5.1~ds-1
*nsis 		nsis 			Available 	3.08-2
pkg-config 	pkg-config 		Installed 	0.29.2

* - Optional Dependency

Diagnosis
---------
Your system is ready for Wails development!
Optional package(s) installation details: 
  - docker: sudo apt install docker.io
  - nsis: sudo apt install nsis



If Wails is useful to you or your company, please consider sponsoring the project:
https://github.com/sponsors/leaanthony



### Additional context

Thank you for Wails. Its value is ramping up!!
@mholt mholt added the Bug Something isn't working label Nov 3, 2022
@leaanthony
Copy link
Member

Hi 👋 It only injects them on the first page at startup. I guess it was designed with SPAs in mind. One issue I can foresee is that there's an event at the bottom of the runtime to indicate it's loaded... How would that work per page? Maybe it just does.... I'd need to think about it a bit more.

Adding a config to just add it to any HTML file being served up would be easy enough I guess. We need to consider what that means for a custom assets handler.

@stffabi
Copy link
Collaborator

stffabi commented Nov 3, 2022

Maybe adding something more configurable would be interesting, like a map of paths of html files where the runtime should be injected.
I think a more explicit way would be great instead of just injecting everywhere. I don't know if that would be feasible in your case @mholt?
Or we could register a predicate handler which should return a boolean depending on the http.Request and indicate if the runtime should be injected. Then there could be helpers for strict paths or if someone want's something very special he could do whatever he wants, or just return true to inject into all html files.

We need to consider what that means for a custom assets handler.

That should work already out of the box, since the injection is done as the very last step before serving the content to the WebViews. It already does injection of the runtime to the index.html even when content has been served by the custom assets handler.

@leaanthony
Copy link
Member

Yeah, the user defined function approach sounds great.

@mholt
Copy link
Contributor Author

mholt commented Nov 3, 2022

Sure, or -- honestly, just adding the scripts myself is nice and easy to understand. It'd have to be documented, but I like having that control and transparency.

Could the 2 files be combined into 1?

One issue I can foresee is that there's an event at the bottom of the runtime to indicate it's loaded... How would that work per page?

I typically run all my code after the page has loaded, like:

function ready(fn) {
	if (document.readyState != 'loading') fn();
	else document.addEventListener('DOMContentLoaded', fn);
}

And according to MDN, this event happens after deferred scripts :

The DOMContentLoaded event fires when the HTML document has been completely parsed, and all deferred scripts (<script defer src="…"> and <script type="module">) have downloaded and executed. It doesn't wait for other things like images, subframes, and async scripts to finish loading.

(Although, on reading this, I have just learned about defer which maybe I can use instead of wrapping all my code in ready()! That might be very nice.)

Does that help?

Personally, I'd be OK with auto-injecting into any HTTP response with a text/html content-type. Or just doing it myself. (Does it hurt to inject the scripts on pages where they aren't used?)

@leaanthony
Copy link
Member

I guess I was thinking of the scenario where currently people treat that as an "init" event and start modifying/loading state. This may have consequences if called multiple times.

I'm erring on a multipronged approach:

  • Have a preference, say RuntimeInjection that defaults to Initial Page Only - the current behaviour (no nasty surprises).
  • Other options: All html files and Custom which uses a predicate.

Thoughts?

@mholt
Copy link
Contributor Author

mholt commented Nov 29, 2022

Sure, that'd probably work for me at least. It does sound like more work, but I suppose the flexibility will be nice.

@stffabi stffabi self-assigned this Dec 12, 2022
@stffabi
Copy link
Collaborator

stffabi commented Dec 12, 2022

I've started working on this, since we would need that also for multi window assets.

Currently I took a simple approach to inject it on all index.html served and when for a directory request a html file is served.

I think that should solve this for multi window assets and also in your case @mholt

What are your thoughts on this approach?

@mholt
Copy link
Contributor Author

mholt commented Dec 12, 2022

I suppose that's fine; although, any thoughts on my 3 proposed solutions in the OP? I'm not sure about implementation complexity but in theory they sound simple and consistent.

@stffabi
Copy link
Collaborator

stffabi commented Dec 12, 2022

Regarding your proposed solutions my very personal thoughts:

  1. Inject those on all HTML pages

Personally I wouldn't prefer to include it in all html files. Maybe a user/framework uses html snippets which would be fetched and included dynamically, which would then probably fail to be loaded.

  1. Don't inject them on any pages (make the user do it in their HTML)

I think that would break all existing apps when not having a config option. And users would need to know the paths on the AssetServer to those files, which might change in the future.

  1. Since the user already has to import App.js to access bound methods, maybe just include/import those files as part of that, so it can happen implicitly?

I'm not sure this will work for plain JS projects, which don't have any frontend build process to have them included/imported. Wouldn't that also tightly couple the frontend to the backend regarding the low-level IPC communication channel?
Low-level IPC can now be WebSocket based or in-process. For the DevServer when accessing with the local browser it's WebSocket, this would also be needed for a discussed hybrid/server #1652 mode. Or it can be in-process when the frontend is loaded from the WebView. This can currently be changed dynamically by the AssetServer, which wouldn't be possible anymore when it's included/imported as part of the frontend build process. At least not without dynamically deciding this in the IPC javascript layer.

@mholt
Copy link
Contributor Author

mholt commented Dec 12, 2022

Those are good counter-points.

Although for (1), maybe only inject on HTML documents that have a <head> ... </head> section? Snippets shouldn't have that, I'd think.

In any case, your solution could be fine too. Either way :)

@leaanthony
Copy link
Member

Another option came to mind: we only care about injecting it when the webview navigates to a new page. I believe there is a signal for this for each webview. Perhaps that could be used? I believe that would prevent the injection in XHR loaded snippets.

@mholt
Copy link
Contributor Author

mholt commented Dec 12, 2022

That sounds promising as well!

@stffabi
Copy link
Collaborator

stffabi commented Dec 12, 2022

That would relatively tight bound the AssetServer to the WebView, currently the assetserver has no knowledge of the webview. Which I sort of like. One would need to correlate that event with the first load of a custom resource event.

Furthermore I currently have no idea how that navigation could be detected when an external DevServer is used and a browser is used for developing. Or how this would work with a hybrid/sever mode.

Saying it's auto-injected in any index.html is an easy constraint which is pretty error free to be implemented and easy explainable to users too. Seems to me like the generalisation of the current well known behaviour.

@leaanthony
Copy link
Member

leaanthony commented Dec 12, 2022

Yeah external Dev server is a good observation. Initially I thought we could just add a query param to the URL from the webview but yeah, external dev server would not work.

I'm ok with the "any index.html" approach. How about we have a config option that defaults to this?

AssetServer: {
   RuntimeInjectionMatchers: [
     "index.html",
   ],
},

We could just do a suffix match on it? (I'm open to better names 🤣)

EDIT: there is another option which we kinda had but in reverse: add a meta tag in your HTML to indicate that you want to inject the runtime and IPC. This would work with any page and plain HTML projects

@stffabi
Copy link
Collaborator

stffabi commented Dec 12, 2022

Yeah we could do that. It would also be injected now if you request '/test/' which is a folder and that folder contains an index.html. So we would probably also need to add '/' to that runtime matchers if you want to have it injected when requesting '/' or '/test/'. Since we can't do http redirects to redirect those requests to '/index.html' or '/test/index.html'.

Meta-Tag has one big drawback in my point of view. This completely defeats having the ability to stream any html page directly to the Webview without buffering it to determine if the file contains the meta tag. Or one would need sniffing a small portion of the file and one could probably miss the meta-tag.

@stffabi
Copy link
Collaborator

stffabi commented Dec 12, 2022

I've have code ready which now always injects into suffix / and suffix /index.html and if the content-type of the result is text/html. I think we also need the / if an external dev server is used which automatically would serve /test/index.html when requesting /test/, what we couldn't detect.

@leaanthony do you want to add the matchers, which defaults to

AssetServer: {
   RuntimeInjectionMatchers: [
     assetserver.PathSuffixMatch("/"),
     assetserver.PathSuffixMatch("/index.html"),
   ],
},

Or should we go with the current hardcoded approach and add that later if we need it?

@stffabi
Copy link
Collaborator

stffabi commented Dec 13, 2022

I've published a draft PR #2203 which contains all common code that is needed to dynamically inject into multiple html files depending on a path precondition.

At the moment the PR hardcoded injects into path suffixes "/" and "/index.html". It could easily be enhanced to support the mentioned RuntimeInjectionMatchers either now or at a later point in time.

@leaanthony
Copy link
Member

I'd say let's make it configurable after 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants