Closed mrksbnch closed 7 years ago
yes, I have to replace with root.KUTE
to link plugins properly, also coming with the next update.
Would you care for some proper testing after update? I can assign you for that.
Sure, I need to use this library for a project anyway. So, I could test if it works with rollup
, browerify
etc.
Please update from npm and let me know how it goes.
Sorry, I didn't have time today, I'll test it either tomorrow or within the next few days. Thanks!
So, I just updated to 1.5.9
, but I got the same error. As far as I can tell, the problem is still the same. kute-svg.js
is looking for KUTE
on the root object, which is not the case for AMD or CommonJS environments.
IDK @mrksbnch I am open to suggestions.
Do you think we can use something like var g = global || window;
or maybe var g = this || window;
can you test if that's gonna work?
I hope @RyanZim can have a look and provide a clue.
How about this solution?
@thednp Normally, this UMD wrapper for the plugins should work (I haven't tested this yet, though):
(function (root, factory) {
if (typeof define === "function" && define.amd) {
define(["kute.js"], factory);
} else if (typeof exports === "object") {
module.exports = factory(require("kute.js"));
} else {
root.KUTE.PLUGIN_NAME = factory(root.KUTE);
}
}(this, function (KUTE) {
// Now you should be able to use `KUTE.pp` or `KUTE.prS` etc.
}));
I'm not sure if you need to export anything for the plugins at all. If not, you can just get rid of module.exports
and root.KUTE.CHANGE_ME
in the code above.
OK but can you please test this?
@thednp Sure, will do later today.
I would personally go the #38 route (major version bump). Plugin-based architectures don't play well with CommonJS, especially when the plugins modify the core libraries functionality.
Plugin-based architectures don't play well with CommonJS, especially when the plugins modify the core libraries functionality.
As for KUTE.js some plugins only extend the KUTE object, not override functionality.
@thednp If you want to fix this issue before version 1.6.0, my code above works for me. Like I said, I don't think you need to export anything from the plugins. So, I can also confirm that this works:
(function (root, factory) {
if (typeof define === "function" && define.amd) {
define(["kute.js"], factory);
} else if (typeof exports === "object") {
factory(require("kute.js"));
} else {
factory(root.KUTE);
}
}(this, function (KUTE) {
I also changed line 26
var g = window, K = g.KUTE /* ... */
to
var K = KUTE /* ... */
However, I got another error on line 242, but this is probably related to #35:
Uncaught TypeError: Cannot read property 'length' of undefined
Any ideas?
Just to clear things up: If I just import KUTE.js and the SVG plugin (with the improved UMD wrapper), I don't get any errors. I just got the last error because I tried to morph two paths:
KUTE.to('#nav-morph-init', {
path: '#nav-morph-init',
}, {
path: '#nav-morph-open',
}).start();
So, it's not strictly related to this issue, but more to #35.
Your above example should be
KUTE.fromTo( // proper tween constructor method to be used, pay attention
'#nav-morph-init', // element
{ path: '#nav-morph-init'}, // start path
{path: '#nav-morph-open'} // end path
).start();
or
KUTE.to( // proper tween constructor method to be used, pay attention
'#nav-morph-init', // element
{path: '#nav-morph-open'} // end path
).start();
I think that's why you get an error, also due to the fact that the script found no element in your HTML.
About your UMD version, probably related to the issue it's about the interpolate functions that are exported to the window
object for performance reasons.
With versions 1.5.0 - 1.5.9 I've done some experiments with regards to performance and the fastest seems to be the usage of window
, but I am also considering creating a separate object to have it's own scope with only active tweens, only interpolate and easing functions used by active tweens and other immediate needed functions or variables.
FYI: I would love to have some coding partners to talk to, brainstorm, exchange ideas in the following spectrum of interests:
I've done some experiments with regards to performance and the fastest seems to be the usage of window
I can't see how that can be that much faster, that is micro-benchmarking IMO. A good read on that subject: https://github.com/getify/You-Dont-Know-JS/blob/master/async%20%26%20performance/ch6.md (If you have time, the whole series is a great read BTW).
Using window
runs the risk of your data being overwritten by other peoples' code.
@RyanZim I am totally against pollution of the global scope but a small little object window._Animation
won't hurt anyone. Possible structure:
var AnimationObject = window._Animation = {
update: function(){},
ticker: function(){},
tick: 0,
domUpdate : {}, // DOM update functions for each property used by active tweens
easing: {}, // easing functions used by active tweens
interpolate: {}, // interpolate functions used by active tweens
tweens: [] // active tweens
}
would be better than populating in the KUTE object, because object lookup access speed and stuff like such.
Sounds good. I'm not really sure why using window
should be faster. But then again, snap.js
is doing the same. So there are probably some advantages.
Personally, I think it's ok to use window.KUTE
if developers just include the script on the page (e.g. with the <script>
tag). But If I import the module (import KUTE from 'kute.js'
) in one file, I wouldn't expect anything to be added to window
. Like I said, snap.js
(window.Snap
and window.mina
(Snap easing functions)) is doing this and I find this really confusing.
Little off topic: I'll try to morph #nav-morph-init
to #nav-morph-open
and then back to #nav-morph-init
(after the first morph is finished). The SVG is included on the page and the #ids are all set. Anyway, I tried to use both of your code examples and got the same error.
@mrksbnch What is wrong with using window
(I mean for the CommonJS/Require stuff)? I am not using window
in the UMD definition, only in the function body. Perhaps you can suggest a better way to suit both browsers and node environments alike.
I now understand that I cannot simply connect KUTE with it's plugins the way I did, your method is probably best, but really can you please explain a little bit what's going on?
What is wrong with using window?
In CommonJS, we expect that everything stays contained inside its own module; that way, there is never any possibility for collision.
would be better than populating in the KUTE object, because object lookup access speed and stuff like such.
I can't see why there would be any speed difference, but I could be wrong. If you can provide actual performance figures/tests, that would be great.
I can't see why there would be any speed difference, but I could be wrong. If you can provide actual performance figures/tests, that would be great.
You have a note on performance on the demo page. Also the performance testing page here. There are almost 2 years testing performance and improving the code, and I figured that's what all major scripts do. Anyways, to put it short: the shorter the "distance/depth" from window
for the scope of the DOM update execution functions the better, and the browser console profiling emphasize this as well.
In CommonJS, we expect that everything stays contained inside its own module; that way, there is never any possibility for collision.
So how can I still use window
or some sort of global from CommonJS, I would guess var g = this || global || window;
So how can I still use window or some sort of global from CommonJS, I would guess
var g = this || global || window
;
If you bundle your files with browserify
or rollup
, this
doesn't always refer to window
. I think in browserify
it binds to the exports object. You can change the context in rollup
, but not in browserify
(AFAIK). I'm not completely sure how webpack
or the other libraries handle this, though.
Anyways, to put it short: the shorter the "distance/depth" from window for the scope of the DOM update execution functions the better, and the browser console profiling emphasize this as well.
Are you referring to the performance of a key lookup in an object? I might be wrong, but if I do...
import KUTE from 'kute.js';
KUTE.to(...);
...I can directly access the KUTE
object. Whereas if you were to bind KUTE
to the window
, it'll probably look like this...
import 'kute.js';
window.KUTE.to(...);
..., and take one more key lookup.
Are you referring to the performance of a key lookup in an object?
Yes, functions like ticker, or those built dynamically for DOM updates, like DOM.transform
.
Take this example: (root is the window
, this
or global
)
root.dom.transform
is a level 2 'distance' from the global objectroot.KUTE.dom.transform
is a level 3 'distance' from the global objectIn any case, no matter what, the first will execute faster because there is less object traversing by a level of one, and that is the nature of Javascript really, no matter what node or browser you're working with.
Please tell me: what are this
and global
in node environments?
@thednp Have you ever benchmarked JS object lookup speed? That is one of the fastest areas of JS IIRC. I would like to see some actual figures of the difference between level 2 & level 3 lookups.
I did a little benchmarking: level 3 lookups are 0.00228635622 nanoseconds slower than level 2 lookups.
Studies show that the brain can't perceive speed differences less than 13 milliseconds. Based on that, you would need to do 5,685,903,136 (that's over 5 billion) object lookups to make a speed difference large enough for the brain to perceive! :open_mouth:
Not worth it IMO.
@RyanZim I perfectly agree with you. Remember the point of this is to achieve best possible performance in most situations (even on very high stress level) so I figured it has to be like this:
So it's not just as you tested in your benchmark Ryan, it's a bit more complex, but we aren't spending time with something important here. We should be going forward with what needs to be done and not argue with things that are facts. GSAP does this, @mrksbnch confirmed snapSVG does this, I already know and do this, so we're wasting time.
Can you please think about a way to make node work in the manner I need please?
@mrksbnch this code you posted
(function (root, factory) {
if (typeof define === "function" && define.amd) {
define(["kute.js"], factory);
} else if (typeof exports === "object") {
factory(require("kute.js"));
} else {
factory(root.KUTE);
}
}(this, function (KUTE) {
is for plugins right?
@thednp Yes
Please stay with me on this, I really want it solved once and for all.
So if I do this }(this, function (root,KUTE) {
will I have access to the global object inside the function body?
Hm, like I said, this
is a little problematic for some module bundlers, because it doesn't always bind to the global scope (window
or global
). I think something like this should work in the browser and node, but I'm not sure.
const g = window !== undefined ? window : global;
@RyanZim Do you know if this will this work?
Also @IngwiePhoenix could be of use in this mater.
@mrksbnch I will prepare some new code with your findings, will publish 1.5.91 on npm and you can do some testings ok? But I need you to test CSS, ATTR and Text plugins as well.
@thednp Sure, will do
Before that, please do me a little test in your node env:
console.log(global||window)
and let me know what this outputs in your node.
@thednp This will output the global
object. window
is undefined in node.
good, in that case we use you UMD wrapper and this var g = global||window;
and we should be doing the tests.
OK my tests are ok, please update and test.
I'm afraid this won't work for CommonJS. This line
module.exports = factory(require("./kute.js"));
should be
module.exports = factory(require("kute.js"));
because your npm package name is kute.js
.
OK patching up and please test 1.5.92.
@thednp Did you already update the code on npm?
yes 1.5.92 https://www.npmjs.com/package/kute.js
What's going on?
Thanks, it's working for me now. However, I couldn't test an actual SVG animation (SVG plugin) due to the Cannot read property 'length' of undefined
error, that I still need to figure out. But, like I said, that's a different issue.
That's because the element is not found. The plugin needs to read the SVGElement
's attributes, especially the d
attribute. Also the plugin should be linked in the bottom of your HTML document.
That's what I thought too, but it's in my HTML:
<svg
class="js-nav-morph"
width="100%" height="100%"
viewBox="0 0 300 300"
preserveAspectRatio="none">
<path id="nav-morph-init" d=".."/>
<path id="nav-morph-open" style="visibility: hidden;" d="..."/>
</svg>
I also tested it with snap.js
and it works. I'll create a separate issue after I've checked the code once again. Thanks!
Can you make a repo so I can have a look at it.
Sure, I can provide a reduced test case (probably not right now) and will create another issue for it.
If I try to import KUTE.js in my project...
..., I'll get the following error in the console:
In this case, I was trying to bundle my JavaScript files with
rollup
, but it doesn't seem to work withbrowserify
either.The SVG plugin looks for KUTE.js on the window object, but this is not defined if KUTE.js is imported from an AMD or CommonJS context (UMD wrapper). So, how's this supposed to work in this case? I couldn't find any notes in the documentation.