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

Browser support #288

Merged
merged 26 commits into from
Jul 29, 2016
Merged

Browser support #288

merged 26 commits into from
Jul 29, 2016

Conversation

nfriedly
Copy link
Contributor

@nfriedly nfriedly commented Jul 27, 2016

This still needs a little cleanup before shipping, including:

  • Update readme and examples to new constructor style
  • Some automated in-browser tests
  • More details on which services support CORS and which don't
  • Some time spent berating the services that don't support CORS

But, I'm creating the PR now in hopes of getting some review from @germanattanasio, @jzhang300, @kognate, and anyone else interested in commenting.

Summary of changes

  • I re-organized the code so that all services can be require()'d and instantiated individually - this allows it to be compatible with browserify and also enables smaller bundles that don't include the entire library
    • In the process of this, I created a base class that had the general logic from the previous lib/index.js, but allowed service-specific hacks to be moved into their own files.
  • I changed the order or preference for credentials and added a new option for simple env properties.
  • I gave the Authorization service a little love so that you can now drop in the credentials object from VCAP_SERVICES to the constructor, and then call getToken() without specifying the URL again.
  • I also put the default URL for each service as a static property on it's constructor to make it easy to pass to the Auth service in non-bluemix environments.

Things I didn't do in this PR, but might like to slip into a v2 release:

  • Promises. Ideally, I'd like everything to return Promises instead of streams unless a stream is specifically requested, and possibly not even then unless it actually makes sense (e.g. speech services). This provides potential to build a much lighter-weight client side (particularly if we don't let it support streams at all.)
  • Merging in the Speech JS SDK & renaming this to the JavaScript SDK
  • Removing request-specific features. We lean pretty heavily on request, but being able to switch it out with something like axioz or possibly just using fetch client-side and a pollyfill in node would be handy.
  • Reducing the size of the bundle - I think it weighs in at like 3.7mb right now (uncompressed - it's like 370kb when minified and gzipped, but still...) Dropping request in favor of fetch would help here.
  • Automatically generating a complete client-side bundle for GithubReleases. Pretty easy to do, but I'd like to get the file-size down a bit first.
  • Dropping support for node v0.12. It's near EOL, and would allow us to use a bit of es6 sugar in the library. Not a bit deal to me though.
  • Breaking changes to make default settings match service defaults. [speech-to-text] Change createRecognizeStream() defaults to match service defaults #257 is the only one I know of off the top of my head, but there may be a few others.

Some of this can slip to v3 - I'd be interested in feedback on what you think makes sense for v2 vs waiting on.

Tickets

This PR resolves https://github.ibm.com/Watson/developer-experience/issues/234 and https://github.ibm.com/Watson/developer-experience/issues/311 on GHE.

Also, on this repo, it
Fixes #30
Fixes #225
Fixes #204
Relates to #283

… data news to use base class, added backwards-compatibility layer
…currently having issues, re-enabling tests for things that seem to be working now
Also fixed the tests - they were "completing" before ever checking the results - and added a couple more tests
everything is now hyphenated so you can do:

require('watson-developer-cloud/tone-analyzer/v1')

instead of

require('watson-developer-cloud/tone_analyzer/v1')
@germanattanasio
Copy link
Contributor

Changes look OK for me. Great job btw.

What if we remove the solr-client dependency. We can ask users to install it if they want to use Retrieve and Rank but having it as a dependency for all our users doesn't make sense.

@nfriedly
Copy link
Contributor Author

I don't think I agree about solr-client. If it's necessary to use that part of the library, then it should be included as a dependency automatically.

But, if a user does, for example require('watson-developer-cloud/conversation/v1'), then they will have solr-client installed in their node_modules/, but it won't be loaded at runtime (or included in a browserify bundle).

As for excluding non-CORS services from browserify.. that's not a bad idea, but I think we can punt it for now. (And hope that they all support CORS in the near future...)

@germanattanasio
Copy link
Contributor

makes sense.

@nfriedly nfriedly changed the title [DON'T MERGE YET] Browser support Browser support Jul 29, 2016
@kognate kognate merged commit d236e91 into master Jul 29, 2016
@germanattanasio germanattanasio deleted the browser branch August 21, 2016 01:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
3 participants