innerHtml fails for `svg` element #165

Closed
ianstormtaylor opened this Issue Jul 19, 2012 · 7 comments

Comments

Projects
None yet
5 participants

in Chrome stable, svg element doesn't seem to have an innerHTML method, so lines 425-427 fail because content === undefined:

var content = element.innerHTML; // undefined

if (content.length > 20) {

if instead that line reads:

if (content && content.length > 20) {

then everything works fine.

lennym commented Mar 18, 2013

Just come up against this as well. The problem seems to be in the buster-format dependency, but I can't seem to find a repo for that anywhere to raise a bug. npm suggests it should be at http://gitorious.org/buster/buster-format but there's nothing there.

Contributor

cjohansen commented Mar 21, 2013

The dependency should be changed to formatio, but this will require a slightly more intelligent build script to avoid size ballooning.

Owner

mantoni commented Mar 21, 2013

I had a quick look at this. The dependencies are samsam and lodash, right? The only features that are used from lodash are type checking and _.keys. Type checking is available in Sinon and _.keys is easy enough to implement.

My suggestion to clean this up (possibly for Sinon 2.0) would be to

  • move the type checking implementation from Sinon to samsam and expose it there
  • add type check for arguments to samsam
  • add (and expose) _.keys equivalent implementation to samsam

The benefits would be

  • the lodash dependency would go away leaving samsam as the only dependency of formatio
  • Sinon should use samsam for value comparison anyway (this is where more work would be needed though)
  • the size of Sinon.JS should stay pretty much the same that way

What do you think?

Contributor

cjohansen commented Mar 21, 2013

Sounds good, @mantoni. I think most of this should be doable in a 1.x compatible manner. The reason samsam ended up depending on lodash is that other buster modules does too, so in the end, samsam would most likely be used in an environment where lodash is available. Hadn't considered the "samsam in sinon" use-case.

My initial idea was to include in the build script a step that would build a dedicated (and minimal) lodash, but I think I like your approach better.

Owner

mantoni commented Mar 21, 2013

Cool. The only thing I need now is days with 48 hours :)

Contributor

cjohansen commented Mar 21, 2013

Who doesn't? :)

@mroderick mroderick modified the milestone: 1.4.0 Aug 11, 2014

Contributor

cjohansen commented Nov 17, 2014

Original problem fixed in formatio: busterjs/formatio@ff623a6

cjohansen closed this Nov 17, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment