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

Simplify prototypes for faster execution and simplify bootstrap.native.js packaging #71

Closed
thednp opened this issue Sep 16, 2016 · 52 comments
Assignees
Labels
community question all in for feedback help wanted need your suggestion

Comments

@thednp
Copy link
Owner

thednp commented Sep 16, 2016

I have a new working code for all modules to work a bit faster, it's a bit more simple and I am also considering packing the bootstrap.native.js file different and not just a concatenation of the modules.

The reason is simple: why write same utility function over and over (addClass or isIE) as well as using module definition for each module when in fact an user gets the all in one package to use it on his web pages, without the need to do require, import, etc.

I really need to know what people think about it.

@thednp thednp added help wanted need your suggestion community question all in for feedback labels Sep 16, 2016
@thednp thednp changed the title Simplify prototypes for faster execution Simplify prototypes for faster execution and simplify bootstrap.native.js packaging Sep 16, 2016
@thednp thednp added this to the 1.0.4 milestone Sep 16, 2016
@thednp thednp self-assigned this Sep 16, 2016
@RyanZim
Copy link
Contributor

RyanZim commented Sep 16, 2016

Have you ever looked into https://github.com/substack/node-browserify? This allows you to write your files in the commonjs module format. Then you can run browserify to bundle all of your files in a UMD wrapper.

@thednp
Copy link
Owner Author

thednp commented Sep 16, 2016

No I haven't, I am totally new to this stuff and I don't afford the time for.

@RyanZim
Copy link
Contributor

RyanZim commented Sep 16, 2016

How were you considering packing bootstrap.native.js?

@thednp
Copy link
Owner Author

thednp commented Sep 16, 2016

Perhaps this would shed some light.

@thednp
Copy link
Owner Author

thednp commented Sep 16, 2016

If you can join me on skype, I can totally provide info on what is to be done.

thednp added a commit that referenced this issue Sep 16, 2016
thednp added a commit that referenced this issue Sep 16, 2016
@thednp
Copy link
Owner Author

thednp commented Sep 16, 2016

@RyanZim please take a look at the new bootstrap.native.js file here and tell me if it doesn't make total sense now.

@RyanZim
Copy link
Contributor

RyanZim commented Sep 16, 2016

@thednp I understand what you're trying to do.

A few notes:

  • bootstrap.native.js does not have a UMD wrapper, therefore, it will not work in commonJS environments.
    • bootstrap.native.js assigns variables to window; you should let the UMD wrapper do this.
  • There is no automated way to keep lib/ and dist/ in sync.

Gotta go, discuss more later.

@thednp
Copy link
Owner Author

thednp commented Sep 16, 2016

bootstrap.native.js does not have a UMD wrapper, therefore, it will not work in commonJS environments.

That file is to be used by users with <script src"bootstrap.native.js"> tag as usual, while users with CommonJS/RequireJS environments can build custom bundles with the modules.

@RyanZim
Copy link
Contributor

RyanZim commented Sep 17, 2016

OK, I understand what you're saying, however:

There is no automated way to keep lib/ and dist/ in sync.

Are you sure you really want to copy code back and forth whenever you change something?

@thednp
Copy link
Owner Author

thednp commented Sep 17, 2016

Are you sure you really want to copy code back and forth whenever you change something?

When it comes to npm/requirejs/commonjs I'm never sure, that's why I ask for experienced users on what's best to do so that everybody is happy.

If there's a way to achieve exactly what I've done with the current bootstrap-native.js file I would go for it, and I think the way the file in question is right now serves most needs without the UMD wrapping, and updating it, as you mentioned cannot be in sync with lib folder, will be done manually, but I see no problem with that.

Generally I need help with the following things:

  • when I change files in lib folder, the files in lib/min should be in sync, and get automatically updated and minified (it's really a hustle doing copy paste with jscompress.com 11 times X 2)
  • a way to build the bootstrap-native.js as a bundle exactly how it is right now, but I think you would say it's not possible
  • someone to come with solutions to these issues where I don't have time to invest in

@RyanZim
Copy link
Contributor

RyanZim commented Sep 17, 2016

when I change files in lib folder, the files in lib/min should be in sync, and get automatically updated and minified (it's really a hustle doing copy paste with jscompress.com 11 times X 2)

That is easily possible, I'll try to submit a PR soon (might be next week).

@RyanZim
Copy link
Contributor

RyanZim commented Sep 17, 2016

a way to build the bootstrap-native.js as a bundle exactly how it is right now, but I think you would say it's not possible

If you want both the bundle and the lib files exactly the same as they are now, I don't know of a way to do it.


why write same utility function over and over (addClass or isIE) as well as using module definition for each module

I agree. However, the way you are doing it now, CommonJS users will not get any of those benefits.


A brief overview of the CommonJS module system:

  • Each module consists of a file that looks like this:

    module.exports = function () {
     // Example:
      return 'Hello World!';
    }
    
  • We can write code like this:

    var hello = require('module-name');
    alert(hello());
    // -> 'Hello World!'
    
  • We run this code through browserify, and that outputs a bundle that includes our code and the modules. We include the bundle with a <script> tag. That way, there is only one http request for javascript.

  • Modules written in the CommonJS module style can't be used with <script> so the UMD wrapper was invented.

  • This allows modules to be written in a IIFE and return the value they want to make publicly available.

  • The UMD wrapper assigns this value to window in a browser environment, but assigns it to module.exports in a CommonJS environment.

@thednp
Copy link
Owner Author

thednp commented Sep 17, 2016

Hey what about a wrapper for the bundle I cooked with the current bootstrap-native.js file?

Users can do CommonJS/RequireJS for the bundle OR build custom bundles themselves with the files from /lib folder. This sounds like a great idea to me.

@RyanZim
Copy link
Contributor

RyanZim commented Sep 17, 2016

Hey what about a wrapper for the bundle I cooked with the current bootstrap-native.js file?

That won't work for a few reasons:

  • The bundle attaches things to window, that's not allowed in CommonJS.

  • The bundle exports multiple variables (Affix, Carousel, etc.). The CommonJS version must export an object containing the functions. We use bootstrap.native like this:

    var bs = require('bootstrap.native');
    var element = document.getElementById('myNav');
    new bs.Affix(element, {
      target: '#use',
      offsetBottom: 200,
    });
    

    CommonJS users never use bootstrap-native.js, instead we use https://github.com/thednp/bootstrap.native/blob/master/all.js.

For now, if you're fine with the copy-paste hassle, lets just leave things as they are. I'll try to automate the minification.

@thednp thednp removed this from the 1.0.4 milestone Sep 21, 2016
@RyanZim
Copy link
Contributor

RyanZim commented Sep 21, 2016

@thednp Any reason to keep this open?

@thednp
Copy link
Owner Author

thednp commented Sep 21, 2016

@RyanZim I still need some opinions, as v1.0.4 is already doing what I explained above. Nobody seems to care/want to get involved. Perhaps users' silence is a sign of approval (because my idea was brilliant in the first place)?

I am thinking: what if we create a different repository only for npm stuff and the current master have it do all things outside npm and UMD? What do you think?

@dgrammatiko
Copy link

I am thinking: what if we create a different repository only for npm stuff and the current master have it do all things outside npm and UMD? What do you think?

I don't use UMD so for me that would be a 💯 The only thing I don't really know is the effort to achieve that and if that worth it

@RyanZim
Copy link
Contributor

RyanZim commented Sep 21, 2016

I am thinking: what if we create a different repository only for npm stuff and the current master have it do all things outside npm and UMD? What do you think?

👎 Keeping bootstrap-native.js and lib/ in sync is enough trouble. Trying to keep two repos in sync is double trouble.

@thednp
Copy link
Owner Author

thednp commented Sep 21, 2016

Good now, we have some feedback.

@RyanZim
Copy link
Contributor

RyanZim commented Sep 21, 2016

The other crazy idea I just though of:

Is making separate files for each module really needed? Does anyone use the files in lib/ directly? If not, I could write a single UMD wrapper for the whole library and we could edit all the source code in one file.

@thednp
Copy link
Owner Author

thednp commented Sep 21, 2016

That's what I am also unsure, I was thinking having them (/lib folder files) separate and some build tools would help each developer build their custom builds similar to what original Bootstrap build tools have. At least that was the purpose of this deleted gulp file.

For instance I don't use carousel, affix, scrollspy and button, so a custom build is a bliss, because an entire lib would overkill the purpose.

@RyanZim
Copy link
Contributor

RyanZim commented Sep 21, 2016

an entire lib would overkill the purpose.

Perhaps others think differently, but bootstrap + jQuery slim is about 59.8KB. With that size, one could see making a custom build. bootstrap.native is only 33KB, so this is less important.

However, if you want to keep the separate files, I'm thinking about the possibility of developing a fairly complex build script that would reduce boilerplate UMD wrappers for both npm and <script> users.

@thednp
Copy link
Owner Author

thednp commented Sep 21, 2016

The later.

@RyanZim
Copy link
Contributor

RyanZim commented Sep 21, 2016

OK, let me think over the best way to do it; might be a couple of days.

@thednp
Copy link
Owner Author

thednp commented Sep 21, 2016

You can write a bullet listed plan that we can discuss, things come from all directions I am sure of that.

@RyanZim
Copy link
Contributor

RyanZim commented Sep 21, 2016

OK, here is a quick proposal for bundling:

  • We remove all the UMD wrappers in lib/.
    • Each module declares a variable directly into whatever scope it is run in. (i.e. affix-native.js would do var Affix = function(element,options) { ... } directly, without an IIFE). I propose that this point is edited, see below.
    • We strongly discourage prohibit the direct usage of the modules in lib/.
  • We move all the utility functions to a separate file (probably placed in lib/).
  • We make a build script that concatenates the modules inside of a UMD wrapper.
    • The UMD wrapper will look something like this:
(function (root, factory) {
    if (typeof define === 'function' && define.amd) {
        // This part is optional, and would add AMD support, 
        // which bootstrap.native currently doesn't have.
        define([], factory);
    } else if (typeof module === 'object' && module.exports) {
        // CommonJS-like:
        module.exports = factory();
    } else {
        // Browser globals (root is window)
        var bsn = factory();
        root.Affix = bsn.Affix;
        root.Alert = bsn.Alert;
        // ... continue for each module
  }
}(this, function () {


    // Include bootstrap.native lib/ files here


    // This is an oversimplification, we can only return values
    // from the files that are included in the bundle:
    return {
       Affix: Affix,
       Alert: Alert,
       // ... continue for each module included
    };
}));
  • The build script would bundle all the modules by default, but allow for excluding modules.
    • I would consider adding a way to wrap just one module (without needing to explicitly ignore everything else).
  • The script would also minify the bundle (we could make this optional).

With this system, we would no longer need the lib/min/ directory, since no one would be using the lib/ modules directly.

Perhaps we could consider adding a modules/ directory to dist/ to contain the UMD-wrapped individual modules (Just an idea; would make my job easier if we didn't do that, but it is possible).

Please don't try to start implementing any of these changes; I'll implement them all at one time when this proposal is approved.

@thednp
Copy link
Owner Author

thednp commented Sep 21, 2016

I like what I see in the code area but I will think about the rest.

@RyanZim
Copy link
Contributor

RyanZim commented Sep 23, 2016

Each module declares a variable directly into whatever scope it is run in. (i.e. affix-native.js would do var Affix = function(element,options) { ... } directly, without an IIFE).

Thinking about this a little more, this layout is asking for variable naming conflicts.

Revision to proposal:

Each module runs inside an IIFE, but declares its main variable into whatever scope it is run in. Example:

var Affix;
(function () {
  Affix = function(element,options) { // This refers to the outside-scope Affix
   //AFFIX DEFINITION
  }
  // AFFIX DATA API
})();

This keeps the data api variables contained in the IIFE.

@thednp
Copy link
Owner Author

thednp commented Sep 23, 2016

I don't like that kind of wrapping, perhaps that wrapping could be done inside the Affix function.
Not that I really know, I really love the way D3.js is organized. https://github.com/d3/d3

@RyanZim
Copy link
Contributor

RyanZim commented Sep 23, 2016

@thednp The issue with my initial proposal is that in the bundle, all the data apis will be running in the same scope (resulting in variable naming collisions). You could do the wrapping differently than above, just so the data api's variables aren't all running in the same scope. Another way to write modules (I sort of prefer this form):

var Affix = function(element,options) {
  //AFFIX DEFINITION
}
(function () {
  // AFFIX DATA API
})();

Another option would be to implement all of the data api's in a separate module. That module could use a single IIFE, and none of the other modules would need to. This would be a slightly fragmented approach 👎, but would result in less IIFEs. Nevermind, I wasn't thinking straight here.


D3 uses RollupJS; I haven't tried it, but I plan to on an upcoming project. RollupJS is actually a module bundler for the ES15/ES6 module format (which hasn't been natively implemented in any browser or runtime yet).

@thednp
Copy link
Owner Author

thednp commented Sep 23, 2016

Fascinating stuff.

@RyanZim
Copy link
Contributor

RyanZim commented Sep 23, 2016

So what are your thoughts on bundling here?

@thednp
Copy link
Owner Author

thednp commented Sep 23, 2016

My mind is full of noise, why don't you do a fork we can work on together and when we have all things working perfect we can pull it to our master here.

@RyanZim
Copy link
Contributor

RyanZim commented Sep 23, 2016

SGTM, won't get to work much on it until Monday. My fork is https://github.com/RyanZim/bootstrap.native. I will be working on a branch named bundle.

Warning: I rebase and force-push quite often. Beware of this if you clone my fork!

@thednp
Copy link
Owner Author

thednp commented Sep 23, 2016

I have a question regarding UMD and performance. You see I'm developing the KUTE.js and I am thinking about a way to define a custom global scope outside UMD wrapper, something to work fast with native functions.

Have a look at this:

var KGLOBAL = {};
(function (factory) {
  if (typeof define === 'function' && define.amd) {
    define([], factory); // AMD. Register as an anonymous module.
  } else if (typeof exports == 'object') {
    module.exports = factory(); // Node, not strict CommonJS
  } else {
    // Browser globals    
    window.KUTE = window.KUTE || factory();
  }
}( function () {
  "use strict";
  var K = window.KUTE, scope = window.KGLOBAL;

  // build K|KUTE object with stuff
  // build scope object with stuff

  return K;
}))

What do you think? Is this doable/legal/working/possible? I am all ears!!

@RyanZim
Copy link
Contributor

RyanZim commented Sep 23, 2016

Is this doable/legal/working/possible?

A few problems:

var KGLOBAL = {}; Never declare anything outside of the UMD wrapper, CommonJS (or AMD) do not allow any global variables.

var K = window.KUTE Not sure what the purpose of this line is, window.KUTE should be undefined (If it isn't, you've broken UMD rules someplace).

Not sure why the scope needs to be global, just do var scope = {}; in your UMD wrapper.

@thednp
Copy link
Owner Author

thednp commented Sep 23, 2016

Not sure why the scope needs to be global, just do var scope = {}; in your UMD wrapper.

You mean here?

(function (factory) {
  if (typeof define === 'function' && define.amd) {
    define([], factory); // AMD. Register as an anonymous module.
  } else if (typeof exports == 'object') {
    module.exports = factory(); // Node, not strict CommonJS
  } else {
    // Browser globals    
    window.KUTE = window.KUTE || factory();
   var scope = {}// HERE??????????????
  }
}( function () {
  "use strict";

  return K;
}))

@RyanZim
Copy link
Contributor

RyanZim commented Sep 23, 2016

(function (factory) {
  if (typeof define === 'function' && define.amd) {
    define([], factory); // AMD. Register as an anonymous module.
  } else if (typeof exports == 'object') {
    module.exports = factory(); // Node, not strict CommonJS
  } else {
    // Browser globals    
    window.KUTE = window.KUTE || factory();
  }
}( function () {
  "use strict";
   var scope = {}// HERE!
  return K;
}))

@thednp
Copy link
Owner Author

thednp commented Sep 23, 2016

var K = window.KUTE Not sure what the purpose of this line is, window.KUTE should be undefined (If it isn't, you've broken UMD rules someplace).

You are right about that. So I am going to export stuff to global var g = window because I'm getting the best performance scores.

@RyanZim
Copy link
Contributor

RyanZim commented Sep 23, 2016

So I am going to export stuff to global var g = window

Look at this:

console.log(window.test); // undefined
(function () { // It doesn't matter if this is an IIFE or UMD wrapper (or any other function).
  var g = window;
  g.test = 'Hello World!';
})();
console.log(window.test); // "Hello World!"
console.log(test); // "Hello World!"
// Oops! made a global!

CommonJS (or AMD) do not allow any global variables.

You can read properties from window (like window.clientHeight), but you cannot add or modify properties to be UMD compliant.

The only place you should export stuff to global is in the UMD wrapper itself: window.KUTE = factory();.

BTW, window.KUTE = window.KUTE || factory(); is overly complex, window.KUTE is undefined, so just do window.KUTE = factory();

@thednp
Copy link
Owner Author

thednp commented Sep 23, 2016

Thank you. I would love to see you around the project with some ideas on how to improve it.

@RyanZim
Copy link
Contributor

RyanZim commented Sep 23, 2016

Sorry, I don't use many animations on my sites, and most of them are simple enough to do in CSS.

@thednp
Copy link
Owner Author

thednp commented Sep 23, 2016

It's not about you using it or not. It's about contributing to the community, and you would only be required with npm/UMD stuff, but I think I managed, I will commit a new version of KUTE.js as soon as I finish all tests.

@RyanZim
Copy link
Contributor

RyanZim commented Sep 25, 2016

@thednp Had extra time today and got to work on it. Check out https://github.com/RyanZim/bootstrap.native/tree/bundle. 🎉

Build script usage:

$ node build.js --help
node build.js [--minify] [--ignore=<modules>...|--only=<modules>...]

Options:
  --minify, -m  Minify output                         [boolean] [default: false]
  --ignore, -i  Omit the given module(s) from the bundle                 [array]
  --only        Only include the given module(s) in the bundle           [array]
  --help        Show help                                              [boolean]

Running without --ignore or --only will compile all the modules.
Writes to stdout

You can run npm run build to automatically compile to dist (will compile both bootstrap-native.js and bootstrap-native.min.js).

lib/utils.js contains the helper functions; they can be used in any module.

Warning: I used some ES6 features in build.js (shorter code). The script should work fine in node v4 or later, but not all the features are supported in browsers yet. 😉

Let me know what you think.

@thednp
Copy link
Owner Author

thednp commented Sep 25, 2016

That looks solid, I will dig into that as soon as I wake up tomorrow.

@thednp
Copy link
Owner Author

thednp commented Sep 25, 2016

Before I go: how would you connect utilities to each lib? Each script needs specific stuff, so no need to use them all if only 2-3 needed? I have an idea but I don't know how it's gonna be implemented with this npm stuff.

@styfle
Copy link

styfle commented Sep 25, 2016

@RyanZim It looks good. You can simplify a lot of the places where you return a boolean by just returning the condition. For example

Before

var allModules = fs.readdirSync('lib/').filter(function (item) {
  if (/-native\.js$/.test(item)) {
    // Is module:
    return true;
  } else {
    // Ignore:
    return false;
  }
});

After

var allModules = fs.readdirSync('lib/').filter(item => /-native\.js$/.test(item));

@thednp
Copy link
Owner Author

thednp commented Sep 25, 2016

I think ignore doesn't work with npm run build -i [modal-native] will just build everything. I believe this only works with node build.js -i [modal-native] or all thing is still is a work in progress or the default answer (=I am total n00b).

@RyanZim
Copy link
Contributor

RyanZim commented Sep 26, 2016

@thednp Passing options directly to npm run build doesn't work (npm run takes options like -s to silence output). The docs refer to using node build.js for custom builds.

Regarding utilities, the minified utils are only around 800 bytes! I think should worry about other optimizations with greater returns.


@styfle Thanks for the tip! Will fix.

@RyanZim
Copy link
Contributor

RyanZim commented Sep 26, 2016

@styfle Fixed! @thednp PR made: #80.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
community question all in for feedback help wanted need your suggestion
Projects
None yet
Development

No branches or pull requests

4 participants