Svg.js: use this context in source of svg.js Incorrectly

[bung87]

https://github.com/svgdotjs/svg.js/blob/f16e4708b6cf0bf9ed939f3fd169776f67f8e0b1/src/umd.js#L12

https://github.com/svgdotjs/svg.js/blob/f16e4708b6cf0bf9ed939f3fd169776f67f8e0b1/src/svg.js#L2

since the umd wrap function use "window" as first parameter,the wraped function should use "window" as this context.

[Fuzzyma]

This is done for the reason that the plugins require a global SVG.
In case of running svg.js in the browser without any module system or bundler this is no problem.
But when you use e.g. require() SVG has to be a global variable. Therefore that's how it is atm.
We hope to remove that "hack" in later versions

[bung87]

If dont use src/svg.js directly,I did't see there is problem with change it be var SVG = window.SVG = function(element) {,since it use umd wrap function and this context has been resolved.but when it comes to webpack ,this context inside factory is unresolved obviously.

[Fuzzyma]

this is always defined. When you dont pass a context, this refers to the global context. And thats what we want

[perry-mitchell]

this is not defined in webpack:

I think it'd be better to fallback to window in the case that this is not available. Even this would be better:

var SVG = (this || window).SVG = function (element) {

EDIT: But as @bung87 said, because you're trying window and then this in the UMD factory, you _could_ just use window in that case..

[perry-mitchell]

I managed to get it working in webpack 3.10 by using imports-loader and define=>false:

module.exports = {
  module: {
    rules: [
      {
        test: require.resolve("svg.js"),
        use: "imports-loader?define=>false"
      }
    ]
  }
};

@Fuzzyma Do you think this would be worthwhile adding to the docs at all? I could make a readme PR if you approve.

EDIT: Seems that this _fix_ still breaks if you import the library into another and build that too with webpack.

EDIT2: I've now moved on from SVG.js to other alternatives, both due to library size and this inconvenience. I'm still interested in seeing this get fixed, however!

[Fuzzyma]

@perry-mitchell sorry to loose you as a user. You are the first one who encountered this issue. Unfortunately we need the this at this point to not break our plugins.
Did you try differenz syntax on require/imports? The transcription to es5 is quite random sometimes

[ValLisetsky]

I have exactly the same issue but with rollup.
this not exists for var SVG = this.SVG = function(element) { line.
please fix as its clearly broken if svgjs used in bundlers

[Fuzzyma]

The problem is that we break working behavior when we just remove it. As I said plugins rely on this syntax (on nodejs) when SVG is not explicitely defined on the global space.
I am afraid you have to wait for 3.0

[perry-mitchell]

@Fuzzyma I hear you on the point of not breaking existing implementations.. but is it so risky to _check_ this before using it? Something like the following could be done to ensure that this error doesn't crop up in bundlers:

var globalRef = (typeof this !== "undefined") ? this : window;

var SVG = globalRef.SVG = function (element) {
  // etc

You can still prioritise this, like I did above - All existing implementations which use this will continue to use it. In the case that it is undefined, like for us, it would simply fall back to window gracefully instead of failing. All it'd be changing is the random error of this being undefined. I would think that this could be classified as a non-breaking change suitable for a minor revision...

[junedchhipa]

I ran into the same issue while using svgjs with a bundler, and I think @perry-mitchell's suggestion might work out well in this case.
@Fuzzyma Would love to hear your thoughts.

[Fuzzyma]

I like that solution. Can you wrap it up into a PR?

[perry-mitchell]

@Fuzzyma I made a PR - #883

[dotnetCarpenter]

Refs:

• https://github.com/svgdotjs/svg.js/issues/659
• https://github.com/svgdotjs/svg.js/issues/660