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

Odd issue with svg #43

Closed
jorgebucaran opened this issue Feb 6, 2017 · 31 comments
Closed

Odd issue with svg #43

jorgebucaran opened this issue Feb 6, 2017 · 31 comments
Labels
bug Something isn't working
Milestone

Comments

@jorgebucaran
Copy link
Owner

The svg clock example live here works fine.

But it fails on CodePen or when run locally.

@jorgebucaran jorgebucaran added the bug Something isn't working label Feb 6, 2017
@jorgebucaran
Copy link
Owner Author

I was able to run the program correctly locally using a simple server. The only instance in which svg fails is on CodePen or when opening an index.html file directly in the browser locally (no server).

This is weird.

@rbiggs
Copy link
Contributor

rbiggs commented Feb 7, 2017

For me, no problem rendering the SVG clock example running on localhost, Chrome and Safari both. But I can't get the setLocation example to work running locally. Maybe something to do with localhost? I see it works fine on the Gomix server.

@rbiggs
Copy link
Contributor

rbiggs commented Feb 7, 2017

I was just looking at the error from the svg clock in Codepen. The error is being thrown by the patch function" var i = e.childNodes[r];. Something about how you're patching the DOM. I am getting the same error with nichoth's example of counters with child elements when I set it to render with a root element. No problem if it just gets appended to the body. nichoth's example. So you might want to look carefully at that patch function for some edge case that is throwing errors.

@tunnckoCore
Copy link

@rbiggs not the only bug coming from patch btw.

@jbucaran when I was playing with the "todo example with components approach" i faced some similar issue or maybe it was kinda "length of undefined" from for (var i = 0; i < node.tree.length; i++) { - that's easy fix, but i'm not sure.

kinda related #39

@selfup
Copy link
Contributor

selfup commented Feb 7, 2017

@tunnckoCore I ran into that length of undefined error but it was just because I didn't close some html tag properly 😂

I was using components approach as well: https://github.com/selfup/hyperapp-idea-box/blob/master/src/views/IdeaBox.js

@jorgebucaran
Copy link
Owner Author

@tunnckoCore 🤔 Any code you could share?

@tunnckoCore
Copy link

tunnckoCore commented Feb 7, 2017

@selfup don't know ;d idea box looks cool, gj

@jbucaran i don't think so...

edit: @selfup, that's single component and single app 😂

@jorgebucaran
Copy link
Owner Author

Ah, components, haven't forgotten about #2. We'll get there too, somewhat, somehow.

@tunnckoCore
Copy link

@jbucaran yep, i'm working on it ;d

@jorgebucaran
Copy link
Owner Author

jorgebucaran commented Feb 7, 2017

But I can't get the setLocation example to work running locally. Maybe something to do with localhost? I see it works fine on the Gomix server.

Router examples using a local server should work. Make sure you are not using a buggy release, there were a few of those recently (like literally the router was broken for a while), during the ES6ish rewrite.

@selfup
Copy link
Contributor

selfup commented Feb 7, 2017

@tunnckoCore I guess it's one component but with multiple views (stateless functional components in React). Using model as the entire application state tree. Just wanted to try it out haha

@selfup
Copy link
Contributor

selfup commented Feb 7, 2017

@jbucaran I am on 0.0.9, I can update to see if I get the same behavior

Update

Everything seems fine with 0.0.11: http://selfup.me/hyperapp-idea-box/

Still amazed at how fast that reloads (refresh render is super fast even from localStorage) mind blown

🎉

Another Update

With 10 "todos" from localStorage after refresh (in incognito with no extensions):

wow-fast

I am using my own localStorage npm module lspi, so if it's somehow slower than you thought it can be because of that 😂

@juxtar
Copy link

juxtar commented Feb 21, 2017

@jbucaran Hey, SVG rendering is failing for me once again on 0.4.2
It was working up until 0.2.1 I believe (before the updates to reducers renaming) and that's what the live example seems to be using.

It breaks when you assign attributes to the svg. Here's a CodePen for a working but useless svg and the broken one.

@jorgebucaran
Copy link
Owner Author

@juxtar IIRC update is still supported in 0.4.2, but I plan to remove it soon.

Thanks, I've updated the examples to use reducers now. Also thanks for figuring out that attributes is what breaks svg in CodePen, that's a hint.

So, to summarize, SVG works when running from your own server (gomix, localhost), but fails in CodePen, specifically, when using attributes.

Could this be like a CodePen/JsFiddle security feature or something?

@juxtar
Copy link

juxtar commented Feb 21, 2017

@jbucaran It's actually failing in my localhost too. Am I doing something wrong? The code (and the error) is the same as in the CodePens.

@jorgebucaran
Copy link
Owner Author

jorgebucaran commented Feb 21, 2017

@juxtar Something obvious is happening on the server that works that we're just missing.

@jorgebucaran
Copy link
Owner Author

jorgebucaran commented Feb 21, 2017

@juxtar You're right, 0.2.1 works, but 0.4.2 doesn't! This bug is fun.

@jorgebucaran
Copy link
Owner Author

jorgebucaran commented Feb 21, 2017

Okay, this is really weird, but it seems SVG elements (namespace) .ns is undefined only in the minified bundle.

Back in 0.2.1 we were still using commonjs, so the example that works is using the source directly.

Maybe this is an uglifyjs issue or how we're using it.

@rbiggs
Copy link
Contributor

rbiggs commented Feb 21, 2017

hmmm... Maybe it's a Browserify issue with the SVG. I've used SVG with JavaScript in the browser without namespaces before.

@jorgebucaran
Copy link
Owner Author

jorgebucaran commented Feb 21, 2017

Could this be a race condition? Very cool bug, should get the hyperapp bug awards this month.

@rbiggs
Copy link
Contributor

rbiggs commented Feb 21, 2017

Maybe something about how Browserify injects things into the browser, combined with iframes on GoMix causing, well, iframe pain.

@jorgebucaran
Copy link
Owner Author

jorgebucaran commented Feb 21, 2017

I'll dig more. Now that I know the cause, the fix is rather simple, but now I have to know why .ns is undefined only when using the uglified bundle.

@rbiggs
Copy link
Contributor

rbiggs commented Feb 21, 2017

Well, I noticed on the failing GoMix svg that's JSX that you have viewbox but no namespace. Try putting it in.

@rbiggs
Copy link
Contributor

rbiggs commented Feb 21, 2017

i'm sure the JSX Babel plugin doesn't add it automatically.

@rbiggs
Copy link
Contributor

rbiggs commented Feb 21, 2017

So i'm getting an error: 155:118TypeError: Attempted to assign to readonly property.

@rbiggs
Copy link
Contributor

rbiggs commented Feb 21, 2017

So after looking at the clock and playing with it locally I think I see why it sometimes doesn't work. x2 and y2 are attributes on the svg line. Therefore we're sometimes getting the error: Attempted to assign to a readonly property. That's because the example is trying to set the value of an attribute dynamically directly on that. Bad boy. Gotta use setAttribute in those cases.

@jorgebucaran
Copy link
Owner Author

jorgebucaran commented Feb 21, 2017

Bug is/was "use strict"! :feelsgood:

Removing "use strict" from the bundle fixed the problem. Or to be more specific, "use strict" causes setting a property in an element like this:

element[name] = value

to fail for read only properties, hence SVG complaining.

Setting the attribute like this:

element.setAttribute(name, value)

works alright.

Possible solutions:

  1. not use "use strict" (perhaps a bad idea) or
  2. avoid setting a property in a SVG element.

@jorgebucaran jorgebucaran added this to the 0.5.0 milestone Feb 21, 2017
@jorgebucaran
Copy link
Owner Author

Fixed in 0.5.0! 💥

@jorgebucaran
Copy link
Owner Author

@rbiggs @juxtar @selfup Clock example now working.

@rbiggs
Copy link
Contributor

rbiggs commented Feb 21, 2017

Yup, I notice after I updated my GoMix to 0.5.0

@selfup
Copy link
Contributor

selfup commented Feb 22, 2017

Ok I take 5.0 seems to be a healthy version :) I will update all my things to 5.0 now and report back!

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

No branches or pull requests

5 participants