Make Cettia work with bundlers such as Webpack #14

Merged
merged 15 commits into from Nov 13, 2016

Conversation

Projects
None yet
2 participants
@DDKnoll
Contributor

DDKnoll commented Nov 3, 2016

Step 1: Copy over old code

For most of the cettia file, I just pulled the code out of that export wrapper and reindented it. I placed it in the src folder to keep it seperate from the

Step 2: Make two seperate versions for browser and node env using Babel.

To separate out the browser and node versions, I used Babel to strip out dead code and export two different versions. Also Babel lets you use all of the new features of javascript! Win win!

In the node environment, you need to polyfill a handful of native window methods. In the browser we need to skip those polyfills. So we add if(browser) statement that can be stripped out:

Initial Code

if (process.env.NODE_ENV !== "browser") {
  window = require("jsdom").jsdom().defaultView;
  window.WebSocket = require("ws");
  window.EventSource = require("eventsource");
}

Replace env variables plugin

if (false) { /** This code is dead! **/
  window = require("jsdom").jsdom().defaultView;
  window.WebSocket = require("ws");
  window.EventSource = require("eventsource");
}

Remove the Dead Code! Final browser.js file does not have those lines.

Step 3: Import the correct version as needed.

// NODE
var cettia = require("cettia-client").default;

// WEBPACK/BROWSERIFY
var cettia = require("cettia-client/browser").default;
@DDKnoll

This comment has been minimized.

Show comment
Hide comment
@DDKnoll

DDKnoll Nov 3, 2016

Contributor

$ npm run build
That will run the eslint build and create the new browser.js file.

$npm run test
Still passes for node environment

Contributor

DDKnoll commented Nov 3, 2016

$ npm run build
That will run the eslint build and create the new browser.js file.

$npm run test
Still passes for node environment

@flowersinthesand

This comment has been minimized.

Show comment
Hide comment
@flowersinthesand

flowersinthesand Nov 4, 2016

Member

unnamed

This is totally awesome! I'll take a look at your fantastic work this weekend and get back to you.

Member

flowersinthesand commented Nov 4, 2016

unnamed

This is totally awesome! I'll take a look at your fantastic work this weekend and get back to you.

@flowersinthesand

This comment has been minimized.

Show comment
Hide comment
@DDKnoll

This comment has been minimized.

Show comment
Hide comment
@DDKnoll

DDKnoll Nov 7, 2016

Contributor

Yeah, I didn't originally make a build step for script tags. But thats easy to add! I just updated the PR with a webpack build command to bundle another version for browsers.

So you can use:

  1. cettia.js for node server-side var cettia = require('cettia-client');. This includes all of the window polyfills.
  2. cettia-bundler.js for webpack/browserify/etc. var cettia = require('cettia-client/cettia-bundler'); This doesn't include all of the dependencies but the bundler will do that for you.
  3. cettia-browser.js for embedded script tags. <script src="path/to/cettia/cettia-browser.min.js" /> This does include all of the dependencies and exports the cettia global object to the window.

The browser embedded version is also doing a uglify step to export a seperate cettia-browser.min.js file.

Does that take care of all of the use cases?

Contributor

DDKnoll commented Nov 7, 2016

Yeah, I didn't originally make a build step for script tags. But thats easy to add! I just updated the PR with a webpack build command to bundle another version for browsers.

So you can use:

  1. cettia.js for node server-side var cettia = require('cettia-client');. This includes all of the window polyfills.
  2. cettia-bundler.js for webpack/browserify/etc. var cettia = require('cettia-client/cettia-bundler'); This doesn't include all of the dependencies but the bundler will do that for you.
  3. cettia-browser.js for embedded script tags. <script src="path/to/cettia/cettia-browser.min.js" /> This does include all of the dependencies and exports the cettia global object to the window.

The browser embedded version is also doing a uglify step to export a seperate cettia-browser.min.js file.

Does that take care of all of the use cases?

@flowersinthesand

This comment has been minimized.

Show comment
Hide comment
@flowersinthesand

flowersinthesand Nov 8, 2016

Member

Hmm, new cettia-browser.js fails to exchange binary events for some reason. I'll dig into this.

Expected - http://jsbin.com/nirocovofe/edit?html,js,console

  <!-- Required only for binary -->
  <script src="https://npmcdn.com/msgpack-lite@0.1.17/dist/msgpack.min.js"></script>
  <!-- Required only for TextEncoder -->
  <script src="https://npmcdn.com/text-encoding@0.5.5/lib/encoding.js"></script>
  <!-- cettia.js -->
  <script src="http://cettia.io/projects/cettia-javascript-client/1.0.0-Beta1/cettia.min.js"></script>

screen shot 2016-11-08 at 8 57 32 pm

Actual - http://jsbin.com/doceyih/edit?html,js,console

  <!-- cettia.js -->
  <script src="https://rawgit.com/DDKnoll/cettia-javascript-client/d14970f056ac3a52eabb5ccae44dcafa9f506298/cettia-browser.js"></script>

screen shot 2016-11-08 at 8 56 39 pm

Member

flowersinthesand commented Nov 8, 2016

Hmm, new cettia-browser.js fails to exchange binary events for some reason. I'll dig into this.

Expected - http://jsbin.com/nirocovofe/edit?html,js,console

  <!-- Required only for binary -->
  <script src="https://npmcdn.com/msgpack-lite@0.1.17/dist/msgpack.min.js"></script>
  <!-- Required only for TextEncoder -->
  <script src="https://npmcdn.com/text-encoding@0.5.5/lib/encoding.js"></script>
  <!-- cettia.js -->
  <script src="http://cettia.io/projects/cettia-javascript-client/1.0.0-Beta1/cettia.min.js"></script>

screen shot 2016-11-08 at 8 57 32 pm

Actual - http://jsbin.com/doceyih/edit?html,js,console

  <!-- cettia.js -->
  <script src="https://rawgit.com/DDKnoll/cettia-javascript-client/d14970f056ac3a52eabb5ccae44dcafa9f506298/cettia-browser.js"></script>

screen shot 2016-11-08 at 8 56 39 pm

+
+ // Determines if the given data contains binary
+ var hasBinary = false;
+ if (process.env.NODE_ENV !== "browser") {

This comment has been minimized.

@DDKnoll

DDKnoll Nov 8, 2016

Contributor

This used to be typeof module.exports === 'object'. Now we can check node env variables directly.

@DDKnoll

DDKnoll Nov 8, 2016

Contributor

This used to be typeof module.exports === 'object'. Now we can check node env variables directly.

@DDKnoll

This comment has been minimized.

Show comment
Hide comment
@DDKnoll

DDKnoll Nov 8, 2016

Contributor

I had to update a few places where it used typeof module.exports to determine whether it was in a node environment or not. Just had to replace with process.env.NODE_ENV !== "browser". Now the buffers are working correctly.

Contributor

DDKnoll commented Nov 8, 2016

I had to update a few places where it used typeof module.exports to determine whether it was in a node environment or not. Just had to replace with process.env.NODE_ENV !== "browser". Now the buffers are working correctly.

@flowersinthesand

This comment has been minimized.

Show comment
Hide comment
@flowersinthesand

flowersinthesand Nov 9, 2016

Member

I've just confirmed that the fix works pretty well - http://jsbin.com/reliyunopa/edit?html,js,console

Does that take care of all of the use cases?

Actually, it doesn't seem to support Asynchronous Module Definition, but it's fine because I think supporting webpack is worth more than supporting AMD.

Would you mind if I squash and merge your work?

Member

flowersinthesand commented Nov 9, 2016

I've just confirmed that the fix works pretty well - http://jsbin.com/reliyunopa/edit?html,js,console

Does that take care of all of the use cases?

Actually, it doesn't seem to support Asynchronous Module Definition, but it's fine because I think supporting webpack is worth more than supporting AMD.

Would you mind if I squash and merge your work?

@DDKnoll

This comment has been minimized.

Show comment
Hide comment
@DDKnoll

DDKnoll Nov 9, 2016

Contributor

Well if you want to do AMD, we could add that as well. Just need to add another webpack build in package.json with the --output-library-target amd flag. Docs on that webpack output.

But everything else looks good to me, otherwise.

Contributor

DDKnoll commented Nov 9, 2016

Well if you want to do AMD, we could add that as well. Just need to add another webpack build in package.json with the --output-library-target amd flag. Docs on that webpack output.

But everything else looks good to me, otherwise.

@flowersinthesand flowersinthesand merged commit 8442ee0 into cettia:master Nov 13, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@flowersinthesand

This comment has been minimized.

Show comment
Hide comment
@flowersinthesand

flowersinthesand Nov 13, 2016

Member

Thanks for the great work! Do you need a release?

Member

flowersinthesand commented Nov 13, 2016

Thanks for the great work! Do you need a release?

@DDKnoll

This comment has been minimized.

Show comment
Hide comment
@DDKnoll

DDKnoll Nov 18, 2016

Contributor

Yeah, just getting around to pulling around the new versino. A release would be great!

Contributor

DDKnoll commented Nov 18, 2016

Yeah, just getting around to pulling around the new versino. A release would be great!

@flowersinthesand

This comment has been minimized.

Show comment
Hide comment
@flowersinthesand

flowersinthesand Nov 22, 2016

Member

@DDKnoll If I run npm run build without installing webpack globally, it throws an error. So I added it locally by running npm install webpack --save-dev and was able to run the build.

However, the generated files are somewhat different from yours, but it passes automated tests on Node and manual tests on browser. It would be nice if you could take a look. #15

Member

flowersinthesand commented Nov 22, 2016

@DDKnoll If I run npm run build without installing webpack globally, it throws an error. So I added it locally by running npm install webpack --save-dev and was able to run the build.

However, the generated files are somewhat different from yours, but it passes automated tests on Node and manual tests on browser. It would be nice if you could take a look. #15

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment