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

Should provide better errors when DOM has unexpectedly changed (e.g. if a library like fontawesome modifies the DOM, I get an invalid memory address or nil pointer dereference instead of human-readable error) #236

Open
nathanhack opened this issue May 20, 2019 · 6 comments
Milestone

Comments

@nathanhack
Copy link
Contributor

nathanhack commented May 20, 2019

It looks like the fontawesome library (v5.8.2) doesn't play nicely with vecty.

The example below shows the check-mark icon in grey but should change to green after typing anything followed by pressing the enter key :
https://play.jsgo.io/119fb23848322ed3323ad3b28614a6674e5a3571

Instead of turning green it produces the following error:

frontend.js:1419 Uncaught Error: runtime error: invalid memory address or nil pointer dereference
    at $callDeferred (frontend.js:1419)
    at $panic (frontend.js:1458)
    at throw$1 (runtime.go:225)
    at Object.$throwNilPointerError (frontend.js:29)
    at Object.$packages.github.com/gopherjs/vecty.HTML.ptr.removeChild (dom.go:650)
    at Object.$packages.github.com/gopherjs/vecty.HTML.ptr.removeChildren (dom.go:616)
    at Object.$packages.github.com/gopherjs/vecty.KeyedList.ptr.remove (dom.go:754)
    at Object.$packages.github.com/gopherjs/vecty.HTML.ptr.reconcileChildren (dom.go:483)
    at Object.$packages.github.com/gopherjs/vecty.HTML.ptr.reconcile (dom.go:225)
    at renderComponent (dom.go:1056)
    at Object.$packages.github.com/gopherjs/vecty.batchRenderer.ptr.render (dom.go:887)
    at f (frontend.js:53)
    at v.$externalizeWrapper (frontend.js:1871)

Any thoughts?

@nathanhack
Copy link
Contributor Author

More or less is related to #204

@slimsag
Copy link
Member

slimsag commented May 27, 2019

Apologies for not taking a look into this yet. From a cursory glance, this definitely looks like it should work today and appears to be a bug in Vecty.

@slimsag slimsag reopened this May 27, 2019
@slimsag
Copy link
Member

slimsag commented May 27, 2019

#204 is about supporting interoperability with JS libraries in a form that is even better than how React does it, AFAIK, or having better documentation for how to do so today.

@slimsag
Copy link
Member

slimsag commented May 27, 2019

The problem here is that FontAwesome applies its own modifications to the DOM structure and violates work that Vecty has done and expects in subsequent renders. In specific, when your Div component is rendering its child div element, FontAwesome hides it and instead inserts its own SVG element in its place:

image

So when Vecty comes back around next time expecting to find the div it previously rendered -- it doesn't and things go kaboom (we should handle reporting such cases better, of course, it is a bug that we do not do this today).

The right way to deal with this is to ensure that the changes FontAwesome makes are isolated to an element solely owned by FontAwesome, which Vecty will treat as an opaque element it doesn't manage.

You can do this by using vecty.UnsafeHTML:

			elem.Div(
				vecty.Markup(
					vecty.UnsafeHTML(`<div class="fas" style="color: green">\uf058</div>`),
				),
			),

Now, while this "works" in the fact that it doesn't explode it will not have the effect you want because you really shouldn't be making use of FontAwesome's JS here. The above gives you the same thing you would get in React if you tried to do this:

https://play.jsgo.io/6042429512bbaab1ed1563f2c215c7e0e815c3ee

What you actually want is something like react-fontawesome but for Vecty: https://fontawesome.com/how-to-use/on-the-web/using-with/react

But that requires basically porting that React code to Vecty. The alternative, which is probably a lot better, is to just use the version of FontAwesome that is based on web fonts and not SVGs so it doesn't require any JS logic to replace divs with generated SVG elements. This works much more nicely, and you don't even need to use UnsafeHTML because FontAwesome isn't modifying the DOM.

Here is an example with what you want: https://play.jsgo.io/dc2b897cc9c7a1b542dc343eda9ad42e348c1888

@slimsag slimsag changed the title fontawesome, invalid memory address or nil pointer dereference Should provide better errors when DOM has unexpectedly changed (e.g. if a library like fontawesome modifies the DOM, I get an invalid memory address or nil pointer dereference instead of human-readable error) May 27, 2019
@slimsag
Copy link
Member

slimsag commented May 27, 2019

I updated the issue title to reflect that we should make this much more obvious when this occurs. Let me know if the above helps or not! :)

@nathanhack
Copy link
Contributor Author

Your response was amazing! And updating the title works for me. I can't disagree that human-readable errors would be great but doing would require a lot of work.

Additionally, thank you for bits on the vecty.UnsafeHTML command; without looking at the code I would have never known it would be treated as an opaque element, there are a few interoperability cases where that will be hugely helpful.

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

No branches or pull requests

2 participants