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

Implement the Observable spec #673

Merged
merged 19 commits into from
Mar 8, 2019
Merged

Conversation

jaidetree
Copy link
Contributor

In reference to #672 this is the same solution with a couple of variations for experimentation and discussion:

  1. Global namespace resolution was split up into a separate file
  2. Nil resolution was split up into a separate file
  3. Moved more of the subscribe consumption logic into the ObservableSubscription class
  4. Moved createObserver into createObserver.js
  5. Separated out isFunction.

What do you think @vqvu?

Copy link
Collaborator

@vqvu vqvu left a comment

Choose a reason for hiding this comment

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

This looks good. Thanks for doing this.

I just have some minor comments.

lib/global.js Show resolved Hide resolved
lib/isFunction.js Outdated Show resolved Hide resolved
lib/observableSubscription.js Outdated Show resolved Hide resolved
lib/observableSubscription.js Show resolved Hide resolved
lib/nil.js Outdated Show resolved Hide resolved
lib/observableSubscription.js Show resolved Hide resolved
@vqvu
Copy link
Collaborator

vqvu commented Mar 8, 2019

Have you verified that this code makes Highland work with Gulp?

@jaidetree
Copy link
Contributor Author

Just did:

Created gulpfile.js

const gulp = require('gulp');
const _ = require('./lib');

gulp.task('list', () => {
    return gulp.src('**/*.js')
        .pipe(_())
        .map(file => file.relative)
        .filter(path => !path.includes('node_modules'))
        .tap(console.log);
});

Executed:

gulp list

Results:

> npx gulp list
[22:21:44] Using gulpfile ~/Projects/highland/gulpfile.js
[22:21:44] Starting 'list'...
Gruntfile.js
gulpfile.js
bench/bench.js
bench/lodash.js
bench/underscore.js
dist/highland.js
dist/highland.min.js
lib/createObserver.js
lib/global.js
lib/index.js
lib/intMap.js
lib/isFunction.js
lib/nil.js
lib/observableSubscription.js
lib/queue.js
lib/readableProxy.js
tasks/docs.js
test/browser.js
test/observableSubscription_test.js
test/test.js
docs/js/foundation.min.js
docs/js/foundation/foundation.abide.js
docs/js/foundation/foundation.accordion.js
docs/js/foundation/foundation.alert.js
docs/js/foundation/foundation.clearing.js
docs/js/foundation/foundation.dropdown.js
docs/js/foundation/foundation.interchange.js
docs/js/foundation/foundation.joyride.js
docs/js/foundation/foundation.js
docs/js/foundation/foundation.magellan.js
docs/js/foundation/foundation.offcanvas.js
docs/js/foundation/foundation.orbit.js
docs/js/foundation/foundation.reveal.js
docs/js/foundation/foundation.tab.js
docs/js/foundation/foundation.tooltip.js
docs/js/foundation/foundation.topbar.js
docs/js/vendor/fastclick.js
docs/js/vendor/highlight.pack.js
docs/js/vendor/jquery.cookie.js
docs/js/vendor/jquery.js
docs/js/vendor/modernizr.js
docs/js/vendor/placeholder.js
[22:21:57] Finished 'list' after 12 s

The fact that we see the output wrapped by gulp's timing logger proves that it subscribed to the observable, processed all the files in a stream, and waited for it to complete.

We are good to go! 🚀

@vqvu
Copy link
Collaborator

vqvu commented Mar 8, 2019

Excellent! Thanks for all the work you put in.

@vqvu vqvu merged commit b1f1c92 into caolan:master Mar 8, 2019
@vqvu vqvu changed the title Subscribe method with separated out nil\global file Implement the Observable spec Mar 8, 2019
@jaidetree
Copy link
Contributor Author

Thank you for all the support and code reviews. I’ve got a few small projects lined up but after I would like to make some progress on v3.

@jaidetree jaidetree deleted the subscribe-with-nil-file branch March 8, 2019 06:47
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