[WIP] - Switch to Rollup #895

Florian-R · 2016-11-16T17:20:08Z

This is one should fixes #894. For now, all tests explode because I couldn't figure a way to compile the code correctly with grunt-mocha-test, but I'd like to grab early comments.

TBD:

Fix tests
Tests on real world app to see it fixes Brunch and Gulp issues
Check for deps require
Check bundle size - Generated with grunt-compare-size :

-255216 build/chaplin.js
-4992 build/chaplin.min.js

If possible, get rid of coffee extensions (see inline comments)
Check if coverage is still OK.

cc @embs @shvaikalesh

This was not needed, the coverage already provide this hooks.

Florian-R · 2016-11-16T17:22:16Z

src/chaplin/composer.coffee

@@ -3,9 +3,9 @@
 _ = require 'underscore'
 Backbone = require 'backbone'


I've just stumble upon the external option of rollup, I'll change this one in a future commit

Florian-R · 2016-11-16T17:24:43Z

src/chaplin.coffee

+  CollectionView
+  Layout
+  View
+}


I'm not super fluent with ES6 modules, almost always used CommonJS. Cannot find a way have the import inlined in the export as were the require. Let me know if this could be done in a nicer way.

ES6 modules can do export { foo } from 'bar.coffee', but that won't help here. Furthermore, we should be careful not break CommonJS users because of default thing.

Florian-R · 2016-11-16T17:28:36Z

Gruntfile.coffee

            'coffee-script/register'
-            'coffee-coverage/register-istanbul'
+            'babel-register'


Tests breaks, because we babel-register had a extension hook but it expect .js file.

A simpler approach could be compile our files before running test in a tmp dir, but it may breaks coverage and will be slower and dirtier.

Florian-R · 2016-11-16T17:31:45Z

src/chaplin.coffee

@@ -1,23 +1,43 @@
 'use strict'

+import Application from './chaplin/application.coffee'


Had to keep extensions, else rollup doesn't resolve the files. See rollup/rollup#1052 (comment). Let me know if your cool with this, else I'll try to use rollup-plugin-node-resolve.

I think we should give rollup-plugin-node-resolve a try, .coffee does not look very nice to me.

paulmillr · 2016-11-16T19:42:54Z

👍

Florian-R · 2016-11-17T15:03:58Z

Added some commits, I got rid of the coffee extensions, and cleaned remaining require.

I've updated my first post, the bundle is way smaller (didn't check gzip tough).

I'd work on getting the tests to pass, in the meantime, @embs could you do a quick check with my rollup-build branch and confirm it fixes your gulp setup? I can confirm this one works fine with Brunch.

shvaikalesh · 2016-11-17T15:05:51Z

Outstanding job, @Florian-R! Code looks so much better now.

shvaikalesh · 2016-11-17T15:26:34Z

src/chaplin/application.coffee

@@ -106,7 +106,7 @@ module.exports = class Application
  # is a replacement for Backbone.Router and *does not inherit from it*
  # directly. It's a different implementation with several advantages over
  # the standard router provided by Backbone. The router is typically
-  # initialized by passing the function returned by **routes.coffee**.


Not sure that we should remove .coffee here.

Wrong search and replace, I'll revert.

shvaikalesh · 2016-11-17T15:47:42Z

Also, we should remove all 'use strict' declarations, because es6 modules are always strict.

vendethiel · 2016-11-17T16:14:25Z

src/chaplin/lib/utils.coffee

@@ -1,5 +1,7 @@
 'use strict'

+import mediator from '../mediator'


No, the require was intended. There's a dependency cycle between utils and mediator.

I'll double check but I'm pretty sure Rollup handle circular dependencies.

oh, amazing then :-)!

Florian-R · 2016-11-17T16:29:03Z

Also, we should remove all 'use strict' declarations, because es6 modules are always strict.

Make sense, done for the src directory for now.

Florian-R · 2016-11-23T17:04:41Z

Added one commits for the test. I had to use a tmp dir to compile the coffee. See benbria/coffee-coverage/issues/82 for the rationale. Let me know if your good with this approach, and I'll tweak the code to avoid a double compilation.

Florian-R · 2016-11-23T17:06:03Z

Gruntfile.coffee

-  grunt.registerTask 'test', 'mochaTest:native'
-  grunt.registerTask 'test:jquery', 'mochaTest:jquery'
+  grunt.registerTask 'test', ['clean', 'instrument', 'coffee:test', 'mochaTest:native']
+  grunt.registerTask 'test:jquery', ['clean', 'instrument', 'coffee:test', 'mochaTest:jquery']


This part need some rework.

Florian-R · 2016-11-23T17:06:47Z

package.json

+    "plugins": [
+      "transform-es2015-modules-commonjs"
+    ]
+  }


Inlined this to avoid a .babelrc

Florian-R · 2016-11-23T17:09:37Z

Gruntfile.coffee

-            'coffee-script/register'
-            'coffee-coverage/register-istanbul'
+            ->
+              global._$coffeeIstanbul = {}


This is ugly, but needed. istanbul add something like

if (typeof _$coffeeIstanbul === 'undefined') _$coffeeIstanbul = {};

in every modules. This breaks badly with babel-register which wrap the code in a function with a use strict pragma.

Edit: In all fairness, this comes indeed from coffee-coverage. I've proposed a fix here benbria/coffee-coverage#82 (comment)

Florian-R · 2016-11-23T17:10:58Z

package.json

    "grunt-cli": "~0.1.13",
    "grunt-coffeelint": "~0.0.15",
+    "grunt-contrib-clean": "1.0.0",
+    "grunt-contrib-coffee": "florian-r/grunt-contrib-coffee",


This is WIP pending a new release of grunt-contrib-coffee. The current version use an old version of coffee-script.

Florian-R · 2017-09-11T08:01:45Z

Closing per #900

Florian-R added 2 commits November 14, 2016 15:18

Update Coffeescript

e36f5fa

Remove coffee-script/register in mochaTest:native

372667f

This was not needed, the coverage already provide this hooks.

Florian-R commented Nov 16, 2016

View reviewed changes

shvaikalesh reviewed Nov 17, 2016

View reviewed changes

vendethiel reviewed Nov 17, 2016

View reviewed changes

Florian-R added 2 commits November 18, 2016 16:05

Switch to rollup

0be2888

test

4337d82

Florian-R commented Nov 23, 2016

View reviewed changes

package.json

"plugins": [

"transform-es2015-modules-commonjs"

]

}

Copy link

Contributor Author

Florian-R Nov 23, 2016

Choose a reason for hiding this comment

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

Inlined this to avoid a .babelrc

Florian-R commented Nov 23, 2016

View reviewed changes

Florian-R mentioned this pull request Nov 24, 2016

coffee-coverage and CoffeeScript 1.11.x (ES2015 modules) benbria/coffee-coverage#82

Open

shvaikalesh mentioned this pull request Sep 11, 2017

Bundle with Rollup #900

Open

Florian-R closed this Sep 11, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] - Switch to Rollup #895

[WIP] - Switch to Rollup #895

Florian-R commented Nov 16, 2016 •

edited

Loading

Florian-R Nov 16, 2016

Florian-R Nov 16, 2016 •

edited

Loading

shvaikalesh Nov 16, 2016

Florian-R Nov 16, 2016

Florian-R Nov 16, 2016 •

edited

Loading

shvaikalesh Nov 16, 2016

paulmillr commented Nov 16, 2016

Florian-R commented Nov 17, 2016

shvaikalesh commented Nov 17, 2016

shvaikalesh Nov 17, 2016 •

edited

Loading

Florian-R Nov 17, 2016

shvaikalesh commented Nov 17, 2016

vendethiel Nov 17, 2016

Florian-R Nov 17, 2016

vendethiel Nov 17, 2016

Florian-R commented Nov 17, 2016

Florian-R commented Nov 23, 2016

Florian-R Nov 23, 2016

Florian-R Nov 23, 2016

Florian-R Nov 23, 2016 •

edited

Loading

Florian-R Nov 23, 2016

Florian-R commented Sep 11, 2017

		@@ -3,9 +3,9 @@
		_ = require 'underscore'
		Backbone = require 'backbone'

		@@ -1,23 +1,43 @@
		'use strict'

		import Application from './chaplin/application.coffee'

		@@ -1,5 +1,7 @@
		'use strict'

		import mediator from '../mediator'

[WIP] - Switch to Rollup #895

[WIP] - Switch to Rollup #895

Conversation

Florian-R commented Nov 16, 2016 • edited Loading

Choose a reason for hiding this comment

Florian-R Nov 16, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Florian-R Nov 16, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paulmillr commented Nov 16, 2016

Florian-R commented Nov 17, 2016

shvaikalesh commented Nov 17, 2016

shvaikalesh Nov 17, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shvaikalesh commented Nov 17, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Florian-R commented Nov 17, 2016

Florian-R commented Nov 23, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Florian-R Nov 23, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Florian-R commented Sep 11, 2017

Florian-R commented Nov 16, 2016 •

edited

Loading

Florian-R Nov 16, 2016 •

edited

Loading

Florian-R Nov 16, 2016 •

edited

Loading

shvaikalesh Nov 17, 2016 •

edited

Loading

Florian-R Nov 23, 2016 •

edited

Loading