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

Add uglify build step #392

Merged
merged 1 commit into from
Nov 18, 2015
Merged

Add uglify build step #392

merged 1 commit into from
Nov 18, 2015

Conversation

svozza
Copy link
Collaborator

@svozza svozza commented Nov 17, 2015

As mentioned in #389 , I've added a minified version to the dist folder and made it part of the build process. I'd still prefer to see the unit tests working on the minified version of the code but it doesn't seem to be that easy to get working.

@vqvu
Copy link
Collaborator

vqvu commented Nov 18, 2015

Probably safe to merge, since presumably we trust uglify and browserify to not break our code...

I'm curious. How are you running the browser tests without using browserify:test-browser?

vqvu added a commit that referenced this pull request Nov 18, 2015
@vqvu vqvu merged commit fafcae9 into caolan:master Nov 18, 2015
@vqvu vqvu added this to the v2.6.0 milestone Nov 18, 2015
@svozza
Copy link
Collaborator Author

svozza commented Nov 18, 2015

No, I was using browserify:test-browser but I was just trying to use Rewireify to inject the miniified version of Highland into that browser.js file.

@svozza svozza deleted the minify-dist branch November 18, 2015 08:41
@vqvu
Copy link
Collaborator

vqvu commented Nov 18, 2015

I see. Can't you just change test/test.js to only require('./lib/index') if there's no global.highland object? Then add the minified bundle to browser.html?

Like this:

diff --git a/test/browser.html b/test/browser.html
index 979c6d0..421e6c5 100644
--- a/test/browser.html
+++ b/test/browser.html
@@ -6,6 +6,7 @@
 </head>

 <body>
+<script src="../dist/highland.js"></script>
 <script src="./bundle.js"></script>
 <script>
 if (!window.nodeunit) {
@@ -21,4 +22,4 @@ if (!window.nodeunit) {

 <hr />
 </body>
-</html>
\ No newline at end of file
+</html>
diff --git a/test/test.js b/test/test.js
index 7c977ab..e1fa946 100755
--- a/test/test.js
+++ b/test/test.js
@@ -6,8 +6,16 @@ var EventEmitter = require('events').EventEmitter,
     concat = require('concat-stream'),
     RSVP = require('rsvp'),
     Promise = RSVP.Promise,
-    transducers = require('transducers-js'),
+    transducers = require('transducers-js');
+
+var _;
+if (global.highland) {
+    _ = global.highland;
+} else {
     _ = require('../lib/index');
+}

 // Use setTimeout. The default is process.nextTick, which sinon doesn't
 // handle.

@vqvu
Copy link
Collaborator

vqvu commented Nov 18, 2015

We can even exclude lib/** when running browserify:test-browser so we're guaranteed to be using the correct highland.

@svozza
Copy link
Collaborator Author

svozza commented Nov 18, 2015

Yep, that's a much better way of doing it. You'll have to forgive me, I have pretty much zero front-end experience so this stuff doesn't come naturally to me. What is odd though is that if I make these changes, the backpressure tests for merge and mergeWithLimit fail and also the generator stream test of parallel.

@vqvu
Copy link
Collaborator

vqvu commented Nov 18, 2015

I tried this with

  • Chrome 46: failures in merge and mergeWIthLimit
  • Firefox 44: failures in merge and mergeWIthLimit
  • IE 11: No failures.

I never see a problem with parallel. The errors for merge and mergeWithLimit seem comes from the https://github.com/defunctzombie/node-process module that browserify includes. Those tests all use sinon timers and _.setImmediate. Our feature detection code for setImmediate looks like this.

// setImmediate implementation with browser and older node fallbacks
if (typeof setImmediate === 'undefined') {
    if (typeof process === 'undefined' || !(process.nextTick)) {
        _.setImmediate = function (fn) {
            setTimeout(fn, 0);
        };
    }
    else {
        // use nextTick on old node versions
        _.setImmediate = process.nextTick;
    }
}
// check no process.stdout to detect browserify
else if (typeof process === 'undefined' || !(process.stdout)) {
    // modern browser - but not a direct alias for IE10 compatibility
    _.setImmediate = function (fn) {
        setImmediate(fn);
    };
}
else {
    _.setImmediate = setImmediate;
}

process.nextTick is always available, so _.setImmediate becomes process.nextTick when the browser doesn't provide setImmediate. It just so happens that IE 11 provides setImmediate, while the newer Chrome and Firefox don't.

My guess is that there's some weird interactions between sinon timers and having two different node-process module (one in bundle.js and one in highland.min.js). Probably one node-process is being used to initialize the currentQueue variable and the other is used to to actually execute process.nextTick, which uses currentQueue. Seems to be a weird anti-aliasing issue where process.nextTick means one thing in one context and another in a different context.

If I change _.setImmediate to be an alias for setTimeout(0) instead of process.nextTick, the tests pass again.

@svozza
Copy link
Collaborator Author

svozza commented Nov 18, 2015

Given that nexttick is deprecated should we remove the fallback and just rely on setTImeout?

@vqvu
Copy link
Collaborator

vqvu commented Nov 18, 2015

I think so.

@svozza
Copy link
Collaborator Author

svozza commented Nov 19, 2015

Cool. I have a branch with all the other changes so I'll add this in as well and raise a PR.

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.

2 participants