Browserify #65

grahamscott · 2015-02-17T15:22:41Z

So that we don't introduce nasty conditionals and reliance on the MutationObserver shim in the adding/removing items code, I've decided to make some major refactorings.

Key changes:

Introduce Browserify to package the plugin as 2 items, selleckt itself and a shim to make it work in IE8/9/10
Introduce Karma as a nice way to run tests locally and via CI

grahamscott · 2015-02-17T15:27:39Z

We'll need to change Travis' setup for this.

stefanbuck · 2015-02-18T09:31:22Z

Cool, look's great. I have a one comment on it.

You are using browserfiy to bundle all source files together and delivery the build just via bower / git-url? No npm publish is involved in your publishing process, right?

If so, this works really well in this case, because selleckt doesn't have any dependencies to other browserfiy friendly modules. Imagine selleckt would have two dependencies, a browserify build would compile all dependencies into the bundle. If now another module of the main application use the same dependencies, the main app would contain duplicate code for the dependencies.

Because of that, a browserify build is required in the main application because browserify is exactly build for that - dependency management. Shipping selleckt with bower is fine, but it should be also available in the npm registry. Which brings us to the next point, resources.

With browserify it's not possible to ship all kind of resources like images, fonts via the npm registry. It works quite good for css with cssify and atomify looks also interesting. But at the end all of these have their issues flexibility, documentation, maintenance.

Yesterday I had a quick look to WebPack and it looks very promising. I need more time to figure out how useful WebPack is or not.

grahamscott · 2015-02-18T09:46:25Z

You are using browserfiy to bundle all source files together and delivery the build just via bower / git-url? No npm publish is involved in your publishing process, right?

Hi @stefanbuck

Yes that's the case right now. I'm definitely open to us publishing to npm later on. Selleckt isn't meant to be bundled with CSS or fonts/images (that's left to the implementor) so I'm not sure we need to worry about the resources issues you mention. However, a PR demonstrating further improvements to this project would be very welcome indeed!

stefanbuck · 2015-02-18T11:01:38Z

Okay, it was more a general comment on the browserify approach. Not directly related to this project.

codazzo · 2015-02-19T15:11:36Z

serve.sh

@@ -1,9 +0,0 @@
-PORT=$1


This is still in the README

In general the README needs to be updated with information about how to see the demo page and how to run the tests

i.e. to debug tests in one browser (Chrome): karma start karma.conf.js --browsers=Chrome --single-run=false

codazzo · 2015-02-19T16:21:32Z

I just realized this PR builds on top of #59 but does not replace it. I will try to review all commits after grahamscott@99549f1. That said I'd like to continue the conversation about MutationObserver in the first PR, as I still have doubts about this approach as I explained in my comment there

codazzo · 2015-02-19T16:31:09Z

lib/LegacySelleckt.js

+    throw new Error('Legacy Selleckt compatability requires the MutationObserver shim');
+}
+
+SingleSelleckt.prototype.DELAY_TIMEOUT = window.MutationObserver._period * 2;


Why is * 2 necessary?

codazzo · 2015-02-19T16:48:58Z

I like how we split up Selleckt into multiple files, this is a great improvement. I also really like Karma 👍.
As far as the specific module format, it's not very important - ultimately selleckt is not used as a module and users are only interested in the builds, which patch window.$. One other option of course would be to turn the single files into ES6 modules, and build transpiled bundles. We can experiment with this in the future.
Apart from the above we should settle on a solution we like and merge #59, then this can be rebased and merged.

- Added Karma as the test runner, which works nicely with browserify, as well as eliminates the need for a local test/index.html and associated mocha.js/css - Use a bundler so that the shim for legacy browsers is packaged separately - Added scripts so that a karma can run locally as well as via Saucelabs (for CI and also to test legacy browsers) - use grunt for managing the build/jshint

Achieved by using conditional comments.

This test measures the popover and gets different results in different browsers, so I've added a 2px tolerance to stop failures that would be acceptable from breaking the build

Travis will now run PRs in Firefox, but master builds will run the main suite in Chrome, Firefox, Safari and IE11 (on Saucelabs) followed by the same suite shimmed with LegacySelleckt for IE8/9/10 (also on Saucelabs)

- SingleSelleckt and MultiSelleckt now use _.defaults for consistency - template parsing is now called template caching and is not side-effecty

codazzo · 2015-02-19T17:15:10Z

README.md

+To target a single browser run:
+
+```javascript
+karma start karma.conf.js --browsers=Chrome --single-run=false


Might be worth adding that this requires a npm install -g karma-cli

codazzo · 2015-02-20T10:52:43Z

Unfortunately bower packages are not "published" (so there's no pre-publish step à la npm) and they do not have a postinstall per-project (they do have support per parent project, see bower/bower#249). One popular solution seems to be having separate repos with the build (see https://github.com/angular/angular vs https://github.com/angular/bower-angular, https://github.com/emberjs/ember.js vs https://github.com/components/ember). This removes the pain of committing builds to source control - a process I think is tedious and error-prone, particularly if not automated.

How about we create a selleckt-build repo or even a branch which contains only the builds and the manifests and gets updated every time a change is pushed to master? A travis build could do either.

spmason · 2015-02-20T10:55:55Z

Could we just update the version script to build a new dist before tagging the version?

The bower config should also be updated to only expose the dist/ folder..

spmason · 2015-02-20T11:00:39Z

Alternatively, a postinstall script on frontend might not be too bad..

codazzo · 2015-02-20T11:01:21Z

Could we just update the version script to build a new dist before tagging the version?

this would mean all versions between tags would have outdated builds... which I'm not 100% a fan of (think of trying to find a bug with bisect)

grahamscott · 2015-02-20T11:06:53Z

this would mean all versions between tags would have outdated builds... which I'm not 100% a fan of (think of trying to find a bug with bisect)

The tests dont use the files in /dist

codazzo · 2015-02-20T11:07:48Z

Sorry I wasn't clear - I was talking about tests in projects that use selleckt

codazzo · 2015-02-20T12:03:10Z

For reference there seems to be a bug that's occasionally breaking the build (possibly related to browserify/factor-bundle#59). Let's keep the builds in dist/ for now and move them to a different branch we identify and fix the issue, so we can have travis fully automate the build process.

Browserify

grahamscott force-pushed the browserify branch 3 times, most recently from e4fa758 to 64c30cc Compare February 17, 2015 20:45

grahamscott mentioned this pull request Feb 17, 2015

Browserify #66

Closed

grahamscott added the enhancement label Feb 18, 2015

grahamscott force-pushed the browserify branch 2 times, most recently from 97e8b38 to e4f4a29 Compare February 19, 2015 12:10

codazzo reviewed Feb 19, 2015
View reviewed changes

grahamscott force-pushed the browserify branch from d2c6795 to b8755fb Compare February 19, 2015 16:01

codazzo reviewed Feb 19, 2015
View reviewed changes

Graham Scott added 9 commits February 19, 2015 17:11

First stab at browserifying everything

563226d

bump jquery version

e8f9a40

Removed wrapper shims

ff80c5c

updated expectations lib

0733278

remove local test runner in preparation for move to Karma

57c9f62

Removed testem in preparation for Karma

c065869

remove ./serve

11ccf8a

Updated jshint config for browserify

aa0b88b

Graham Scott added 14 commits February 19, 2015 17:11

Changed demo page to use the legacy selleckt in IE8/9/10

56228e7

Achieved by using conditional comments.

Added some tolerance to a test that occasionally fails.

e487d14

This test measures the popover and gets different results in different browsers, so I've added a 2px tolerance to stop failures that would be acceptable from breaking the build

Added tests for Legacy Selleckt

d79bd43

removed garbage form bower.json

4ab44f7

added missing dev dependency

6bf9319

Travis should run Karma

9f1436f

updated saucelabs credentials

df3318b

remove sauce connect setting from travis.yml

cff8233

Improve config for Travis

d63a5ad

Travis will now run PRs in Firefox, but master builds will run the main suite in Chrome, Firefox, Safari and IE11 (on Saucelabs) followed by the same suite shimmed with LegacySelleckt for IE8/9/10 (also on Saucelabs)

update README

8b1d04a

Tidied up demo page style

bdd7bd4

Tidied up instantiation

4b52c11

- SingleSelleckt and MultiSelleckt now use _.defaults for consistency - template parsing is now called template caching and is not side-effecty

Remove erroneous lag

e16ca43

New build

6481300

grahamscott force-pushed the browserify branch from e6c10e2 to 6481300 Compare February 19, 2015 17:11

codazzo reviewed Feb 19, 2015
View reviewed changes

Graham Scott added 4 commits February 19, 2015 17:20

Inverted the check for templates being a string

30fcfa5

Added installation of karma-cli to the README

9735999

fixed jshint error

897c523

new build

56ae086

codazzo added a commit that referenced this pull request Feb 20, 2015

Merge pull request #65 from grahamscott/browserify

4eabb58

Browserify

codazzo merged commit 4eabb58 into BrandwatchLtd:master Feb 20, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Browserify #65

Browserify #65

grahamscott commented Feb 17, 2015

grahamscott commented Feb 17, 2015

stefanbuck commented Feb 18, 2015

grahamscott commented Feb 18, 2015

stefanbuck commented Feb 18, 2015

codazzo Feb 19, 2015

codazzo Feb 19, 2015

codazzo Feb 19, 2015

codazzo commented Feb 19, 2015

codazzo Feb 19, 2015

codazzo commented Feb 19, 2015

codazzo Feb 19, 2015

codazzo commented Feb 20, 2015

spmason commented Feb 20, 2015

spmason commented Feb 20, 2015

codazzo commented Feb 20, 2015

grahamscott commented Feb 20, 2015

codazzo commented Feb 20, 2015

codazzo commented Feb 20, 2015

Browserify #65

Browserify #65

Conversation

grahamscott commented Feb 17, 2015

grahamscott commented Feb 17, 2015

stefanbuck commented Feb 18, 2015

grahamscott commented Feb 18, 2015

stefanbuck commented Feb 18, 2015

codazzo Feb 19, 2015

Choose a reason for hiding this comment

codazzo Feb 19, 2015

Choose a reason for hiding this comment

codazzo Feb 19, 2015

Choose a reason for hiding this comment

codazzo commented Feb 19, 2015

codazzo Feb 19, 2015

Choose a reason for hiding this comment

codazzo commented Feb 19, 2015

codazzo Feb 19, 2015

Choose a reason for hiding this comment

codazzo commented Feb 20, 2015

spmason commented Feb 20, 2015

spmason commented Feb 20, 2015

codazzo commented Feb 20, 2015

grahamscott commented Feb 20, 2015

codazzo commented Feb 20, 2015

codazzo commented Feb 20, 2015