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

Bundle with rollup / browserify (ES2015 modules) #40

Closed
mrksbnch opened this issue Oct 2, 2016 · 47 comments
Closed

Bundle with rollup / browserify (ES2015 modules) #40

mrksbnch opened this issue Oct 2, 2016 · 47 comments
Assignees
Labels
Milestone

Comments

@mrksbnch
Copy link

mrksbnch commented Oct 2, 2016

If I try to import KUTE.js in my project...

import kute from 'kute.js';
import 'kute.js/kute-svg';

..., I'll get the following error in the console:

kute-svg.js:26 Uncaught TypeError: Cannot read property 'pp' of undefined

In this case, I was trying to bundle my JavaScript files with rollup, but it doesn't seem to work with browserify 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.

@thednp
Copy link
Owner

thednp commented Oct 2, 2016

yes, I have to replace with root.KUTE to link plugins properly, also coming with the next update.

@thednp thednp added the bug label Oct 2, 2016
@thednp thednp added this to the 1.5.9 milestone Oct 2, 2016
@thednp thednp self-assigned this Oct 2, 2016
@thednp
Copy link
Owner

thednp commented Oct 2, 2016

Would you care for some proper testing after update? I can assign you for that.

@mrksbnch
Copy link
Author

mrksbnch commented Oct 2, 2016

Sure, I need to use this library for a project anyway. So, I could test if it works with rollup, browerify etc.

thednp added a commit that referenced this issue Oct 3, 2016
#39

Also fixing #40

Minor documentation changes
thednp added a commit that referenced this issue Oct 3, 2016
#39

Also fixing #40

Minor documentation changes
@thednp
Copy link
Owner

thednp commented Oct 3, 2016

Please update from npm and let me know how it goes.

@thednp thednp closed this as completed Oct 3, 2016
@mrksbnch
Copy link
Author

mrksbnch commented Oct 3, 2016

Sorry, I didn't have time today, I'll test it either tomorrow or within the next few days. Thanks!

@mrksbnch
Copy link
Author

mrksbnch commented Oct 4, 2016

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.

@thednp
Copy link
Owner

thednp commented Oct 5, 2016

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?

@thednp thednp reopened this Oct 5, 2016
@thednp
Copy link
Owner

thednp commented Oct 5, 2016

I hope @RyanZim can have a look and provide a clue.

How about this solution?

@mrksbnch
Copy link
Author

mrksbnch commented Oct 5, 2016

@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.

#38 will also solve this issue.

@thednp
Copy link
Owner

thednp commented Oct 5, 2016

OK but can you please test this?

@mrksbnch
Copy link
Author

mrksbnch commented Oct 5, 2016

@thednp Sure, will do later today.

@RyanZim
Copy link

RyanZim commented Oct 5, 2016

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.

@thednp
Copy link
Owner

thednp commented Oct 5, 2016

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.

@mrksbnch
Copy link
Author

mrksbnch commented Oct 5, 2016

@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?

@mrksbnch
Copy link
Author

mrksbnch commented Oct 5, 2016

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.

@thednp
Copy link
Owner

thednp commented Oct 5, 2016

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:

  • function memoization
  • code structure and constructor class abstraction
  • generally ideas to improve code performance, quality, usability while keeping it light and simple

@RyanZim
Copy link

RyanZim commented Oct 5, 2016

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.

@thednp
Copy link
Owner

thednp commented Oct 5, 2016

@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.

@mrksbnch
Copy link
Author

mrksbnch commented Oct 5, 2016

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.

@thednp
Copy link
Owner

thednp commented Oct 5, 2016

@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?

@RyanZim
Copy link

RyanZim commented Oct 5, 2016

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.

@thednp
Copy link
Owner

thednp commented Oct 5, 2016

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;

@mrksbnch
Copy link
Author

mrksbnch commented Oct 6, 2016

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.

@thednp
Copy link
Owner

thednp commented Oct 6, 2016

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 object
  • root.KUTE.dom.transform is a level 3 'distance' from the global object

In 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?

@RyanZim
Copy link

RyanZim commented Oct 6, 2016

@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.

@RyanZim
Copy link

RyanZim commented Oct 6, 2016

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! 😮

Not worth it IMO.

@thednp
Copy link
Owner

thednp commented Oct 6, 2016

@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:

  • parse strings, process values to create tween objects
  • store these objects in an object in the root of the page or root of the node environment scope (the tween objects array)
  • when we start ticking (btw ticker is/should also be in that root scope) it should quickly find the objects, easing functions and dom update functions
  • in short there are 4 functions/objects that need to be closest to the global scope (tweens, easings, domupdates, ticker) that need to find each other
  • if you multiply this by a factor of 10-100 those milliseconds start stacking and even to the point where the total time to process (say 500 elements animation) exceed 16ms and overall animation is not 60fps anymore.

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?

@thednp
Copy link
Owner

thednp commented Oct 7, 2016

@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?

@mrksbnch
Copy link
Author

mrksbnch commented Oct 7, 2016

@thednp Yes

@thednp
Copy link
Owner

thednp commented Oct 7, 2016

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?

@mrksbnch
Copy link
Author

mrksbnch commented Oct 7, 2016

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?

@thednp
Copy link
Owner

thednp commented Oct 7, 2016

Also @IngwiePhoenix could be of use in this mater.

@thednp
Copy link
Owner

thednp commented Oct 7, 2016

@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.

@mrksbnch
Copy link
Author

mrksbnch commented Oct 7, 2016

@thednp Sure, will do

@thednp
Copy link
Owner

thednp commented Oct 7, 2016

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.

@mrksbnch
Copy link
Author

mrksbnch commented Oct 7, 2016

@thednp This will output the global object. window is undefined in node.

@thednp
Copy link
Owner

thednp commented Oct 7, 2016

good, in that case we use you UMD wrapper and this var g = global||window; and we should be doing the tests.

@thednp
Copy link
Owner

thednp commented Oct 7, 2016

OK my tests are ok, please update and test.

@thednp thednp closed this as completed in ccb5e6b Oct 7, 2016
thednp added a commit that referenced this issue Oct 7, 2016
@mrksbnch
Copy link
Author

mrksbnch commented Oct 7, 2016

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.

@thednp
Copy link
Owner

thednp commented Oct 7, 2016

OK patching up and please test 1.5.92.

thednp added a commit that referenced this issue Oct 7, 2016
thednp added a commit that referenced this issue Oct 7, 2016
@mrksbnch
Copy link
Author

mrksbnch commented Oct 7, 2016

@thednp Did you already update the code on npm?

@thednp
Copy link
Owner

thednp commented Oct 7, 2016

yes 1.5.92 https://www.npmjs.com/package/kute.js

What's going on?

@mrksbnch
Copy link
Author

mrksbnch commented Oct 7, 2016

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.

@thednp
Copy link
Owner

thednp commented Oct 7, 2016

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.

@mrksbnch
Copy link
Author

mrksbnch commented Oct 7, 2016

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!

@thednp
Copy link
Owner

thednp commented Oct 7, 2016

Can you make a repo so I can have a look at it.

@mrksbnch
Copy link
Author

mrksbnch commented Oct 7, 2016

Sure, I can provide a reduced test case (probably not right now) and will create another issue for it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants