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

Use rollup for builds #47

Merged
merged 14 commits into from
Sep 28, 2018
Merged

Use rollup for builds #47

merged 14 commits into from
Sep 28, 2018

Conversation

GerkinDev
Copy link
Owner

Hey! Here we come to the PR of #45!

I've configured examples for the new built files, & created a rollup config for examples (just run npm run examples. I've also a bit changed the instructions on the ajax page to get rid of any globally installed packages (portable scripts rulz). Everything seems fine.

If you really care about gaining some bytes, it may be useful to change from object-path and use another non-UMD module. Also, this may trigger some trivial errors about rollup not being able to check if set or get is exported by this module, and it requires a bit of extra conf for application build:

https://github.com/pstephan1187/vue-datatable/blob/fe468af64a83575b60ef30f49f50c917684c82cb/examples/rollup.examples.config.js#L11-L19

Btw, about #24, you can now import whatyouwant from 'vue';, as it is excluded from builds. I know you already solved this problem, but hey, it's always good to have & know.

Cheers!

Gerkin added 9 commits September 17, 2018 20:22
Removed `hot` command from `package.json`, as rollup does not support HMR.
`rollup-plugin-terser` is locked @v2.0.2 because of TrySound/rollup-plugin-terser#5

NOTE: No unit test working at this time! I could not check if the config works well on actual applications!
Related to #45. NOTE: `object-path` is an UMD module, that can't be well tree-shaken by rollup. It would be best to use equivalent pure CommonJS/ES modules
The main field now uses the pre-built ES (for modules) version of the package.
The browser-ready bundle is now an IIFE, that does not export anything, and simply register itself in vue.
Replaced objectPath all import to simple `get` import in `src/classes/column`
Deleted `webpack.config.js`
@GerkinDev
Copy link
Owner Author

Hey @pstephan1187 ! Any news on this?

@pstephan1187
Copy link
Collaborator

Awesome! I'm loving that file size! 17Kb compared to the 26 previously is super nice! Also, the build time is so much faster. Really liking that. Nice job

I do have a couple thoughts:

  • I would prefer it if the compiled, browser-ready file was not renamed. I'd like to keep it as vuejs-datatable.js instead of vuejs-datatable.iife.js. That would require updates to be made to the documentation and may require a major version number change for SemVer (not totally sure). Is there a reason to have the .iife?
  • I am curious what the .es.js file is for. For users bundling their app scripts, what benefit do they have using that file and not the index.js file? The compiled example scripts did not see a file size reduction (unless I'm doing something wrong), so I am curious what the purpose of that script is.

Forgive me if my questions seem elementary. I do not spend a lot of time in the UMD/CommonJs/ES6 world. I am a backend developer and have been playing catch-up the last year or so and am still ignorant with all the differences and idiosyncrasies.

Also, the example scripts that have to be compiled are not working (custom theme and AJAX). When opening the examples in the browsers, I am getting a Vue is not defined error. Commenting out line 42 in the rollup.examples.config.js file only introduces more errors.

Thus far, I am really liking the benefits of rollup. Much smaller file sizes and much faster build times are great. Thank you for the work you have done.

@GerkinDev
Copy link
Owner Author

GerkinDev commented Sep 26, 2018

  1. No particular reason for the .iife suffix, I just find it convinient to see what is the module's format without looking into the source code (I've had this problem trying to understand the module format of object-path. . . But its up to you! It's your lib, you're the boss!

  2. The .es is the ESM bundle for your module (you can find the es2015 specs here, which defines the ESM format (WARNING! Big & boring)). Its purpose is to be used using the incoming future of client-side javascript, with ES Modules (see this article). And even if you or your users don't plan to use ES Modules for now (as this feature is still pretty immature), using the ESM has some advantages:

    • It reduce the final application build time. Ok, by just a little second... So this argument is much more for standard than for your use case.
    • It allow you to use modern syntax in your source, without impacting backward-compatibility. As an example, your lib is using es6 syntax (by using, eg, the class keyword), and still ship a code that is compatible with old browsers by downgrading to es5 syntax. So far, this is for your use-case.
      You can extrapolate a bit and consider that you may use other languages to build your module, like Typescript or Coffeescript, and still ship transpiled js code. This is more standard than use-case.
      Anyway, it costs you a couple of secs to generate, and not even 50kb in output files sizes... That would not bloat the download size in the browser anyway.

    So even if this build isn't that important for now, it will probably be the main dist file of your package in 6 months to 2 years. And I highly encourage you to keep it. The question that remains is:

    Should we use .js, .jsm or .mjs ? (Michael Jackson Script)

    I've done some searches and can't really decide... So I'd personnaly tend to fallback on good ol' .js

  3. No problem, I take this opportunity to re-learn myself all that modules-format crap I didn't understand just a couple of months ago. So don't hesitate to ask questions, I'll answer them for both of us ;)

  4. Have you run npm install, npm run production and npm run examples (in that order) ? This will rebuild the required JS files.


I've spotted a few fixes to do anyway. So don't accept the PR for now. I wait for your reply to eventually do changes for 1., 2., or 4. if required.

@pstephan1187
Copy link
Collaborator

I appreciate the good answers. I'll have to do more reading on the different Javascript formats.

  1. Let's drop the .iife suffix. This file is just to allow users who don't want to run build scripts to use datatables in their apps. I don't think they are going to care much about format, they just want it to work.

  2. We can keep this. It sounds like it will make the script compatible across more formats (is that the right term for cjs/es6/umd/etc?).

  3. I checked out the PR, deleted the node_modules folder, ran npm install, then npm run production, then npm run examples. When I run the examples script, I get the following output:

> vuejs-datatable@1.6.0 examples /Users/patrick/Packages/vue-datatable
> rollup -c examples/rollup.examples.config.js


/Users/patrick/Packages/vue-datatable/examples/ajax/app.js → examples/ajax/build/app.js...
(!) Missing shims for Node.js built-ins
Creating a browser bundle that depends on 'tty', 'util', 'url', 'http', 'https', 'assert', 'stream' and 'zlib'. You might need to include https://www.npmjs.com/package/rollup-plugin-node-builtins
(!) Unresolved dependencies
https://github.com/rollup/rollup/wiki/Troubleshooting#treating-module-as-external-dependency
http (imported by node_modules/follow-redirects/index.js, node_modules/axios/lib/adapters/http.js, commonjs-external:http)
https (imported by node_modules/follow-redirects/index.js, node_modules/axios/lib/adapters/http.js, commonjs-external:https)
url (imported by node_modules/follow-redirects/index.js, node_modules/axios/lib/adapters/http.js, commonjs-external:url)
zlib (imported by node_modules/axios/lib/adapters/http.js, commonjs-external:zlib)
assert (imported by node_modules/follow-redirects/index.js, commonjs-external:assert)
stream (imported by node_modules/follow-redirects/index.js, commonjs-external:stream)
tty (imported by node_modules/debug/src/node.js, commonjs-external:tty)
util (imported by node_modules/debug/src/node.js, commonjs-external:util)
(!) Missing global variable names
Use output.globals to specify browser global variable names corresponding to external modules
tty (guessing 'tty')
util (guessing 'util')
url (guessing 'url')
http (guessing 'http')
https (guessing 'https')
assert (guessing 'assert')
stream (guessing 'stream')
zlib (guessing 'zlib')
created examples/ajax/build/app.js in 1.7s

/Users/patrick/Packages/vue-datatable/examples/custom-theme/app.js → examples/custom-theme/build/app.js...
created examples/custom-theme/build/app.js in 709ms

@GerkinDev
Copy link
Owner Author

GerkinDev commented Sep 26, 2018

  1. Since most apps are built using a bundler (webpack, rollup, etc), most users won't see any difference. But it allow to use your lib as module, for those who would try to use the new ESM format.

  2. Those are just warnings, in case the app use the mentioned node libs, and it does not. So it won't cause troubles for the example. Just try it, it will work fine. EDIT: OK sometimes it cause troubles.

Gerkin added 5 commits September 26, 2018 16:38
`iife` build now named `vuejs-datatable.js`
`esm` build now named `vuejs-datatable.esm.js`
`npm run build` is equivalent to `npm run production && npm run examples`
@GerkinDev
Copy link
Owner Author

OK I think it's good to go. Sorry for that: I don't understand why the error you saw wasn't thrown on my environment... Maybe cache problems. :/

Copy link
Collaborator

@pstephan1187 pstephan1187 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks good! All the examples are running perfectly now. Thank you for the work!

@pstephan1187 pstephan1187 merged commit e39f4cf into GerkinDev:master Sep 28, 2018
@GerkinDev GerkinDev deleted the feature/rollup branch October 1, 2018 12:57
GerkinDev pushed a commit to diaspora-orm/diaspora that referenced this pull request Oct 2, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants