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

Convert ActionCable javascript to ES2015 modules with a modern build environment #34177

Merged
merged 4 commits into from Nov 2, 2018

Conversation

Projects
None yet
7 participants
@rmacklin
Copy link
Contributor

commented Oct 10, 2018

Summary

This pull request migrates ActionCable's javascript source code and tooling to mirror that of the more modern ActiveStorage package.

The goals are:

  1. Create consistency in the javascript environments for ActionCable and ActiveStorage
    • The aim is to converge on how each package handles the file structure, linting, build pipeline, etc.
  2. Migrate ActionCable from CoffeeScript to ES2015 and from sprockets //= require directives to ES2015 module syntax
    • This is consistent with the more modern style of ActiveStorage's javascript. Most of the front end community has moved away from CoffeeScript and embraced ES2015+, so it seems like a good idea to modernize ActionCable's front end using the same technologies as ActiveStorage.
  3. Update the ActionCable npm package to ship untranspiled ES2015 source code in addition to the heavier compiled bundle
    • This enables ActionCable users to ship smaller javascript bundles to their visitors with modern browsers. We added this enhancement to ActiveStorage in #31880.

Other Information

As in #31880, I've created a sample repository to demonstrate the benefits of goal (3):
https://github.com/rmacklin/actioncable-es2015-build-example

In that example, the bundle shrinks by 2.5K (23%) when you simply change the actioncable import to point to the untranspiled src, e.g. by setting up an alias:

resolve: {
  alias: {
    actioncable: 'actioncable/src'
  }
}

If you go a step further, by using a fine-grained import to pull in only what your app uses, e.g.:

diff --git a/app/scripts/main.js b/app/scripts/main.js
index 17bc031..1a2b2e0 100644
--- a/app/scripts/main.js
+++ b/app/scripts/main.js
@@ -1,6 +1,6 @@
-import ActionCable from 'actioncable';
+import {createConsumer} from 'actioncable/action_cable';

-let cable = ActionCable.createConsumer('wss://cable.example.com');
+let cable = createConsumer('wss://cable.example.com');

 cable.subscriptions.create('AppearanceChannel', {

then the bundle shrinks by 3.2K (30%).

In addition to allowing smaller bundles for those who ship untranspiled code to modern browsers, including the source code in the published package can be useful in other ways:

  1. Users can import individual modules rather than the whole library
  2. As a result of (1), users can also monkey patch parts of ActionCable by importing the relevant module, modifying the exported object, and then importing the rest of ActionCable (which would then use the patched object).

PR Breakdown

Given the scope of the changes, this is not a small PR. I did my best to include thorough commit messages. Here are the high level chunks of how the PR breaks down:

  1. Convert regular CoffeeScript files to ES2015 (but not ES2015 modules yet) using decaffeinate 61f5ba7~...0398e59
  2. Clean up the converted code cbe65e0~...8ce6987
  3. Convert action_cable.coffee.erb and do more cleanup 8f23df1~...bba49e0
  4. Modernize ActionCable tooling/environment to mirror ActiveStorage's setup 3ae4ebf~...575ce04
  5. Convert ActionCable to ES2015 modules 8936336~...32e5048
  6. Final cleanup and finishing touches 1866d0b~...83fbeee
  7. Include source code in published npm package 13b429a

Unfortunately, splitting off separate PRs for each chunk doesn't work because you have to get all the way to ES2015 modules to leverage the rollup build pipeline. (The build process is broken in the intermediate stages, and there's not an easy way to avoid that given the limitations of sprockets.)


cc @javan - I'd love to hear your thoughts on this, and I have some additional questions and ideas that we could discuss to improve this.


Edit: Goal (3) has been split out to a new PR: #34258

@rails-bot

This comment has been minimized.

Copy link

commented Oct 10, 2018

Thanks for the pull request, and welcome! The Rails team is excited to review your changes, and you should hear from @rafaelfranca (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

This repository is being automatically checked for code quality issues using Code Climate. You can see results for this analysis in the PR status below. Newly introduced issues should be fixed before a Pull Request is considered ready to review.

Please see the contribution instructions for more information.

@rails-bot rails-bot bot added the actioncable label Oct 10, 2018

@javan javan assigned javan and unassigned rafaelfranca Oct 10, 2018

@javan

This comment has been minimized.

Copy link
Member

commented Oct 10, 2018

Exemplary pull request, @rmacklin! I will give this a closer look soon.

@javan

This comment has been minimized.

Copy link
Member

commented Oct 11, 2018

I have some additional questions and ideas that we could discuss to improve this.

I'm all ears :)

@rmacklin

This comment has been minimized.

Copy link
Contributor Author

commented Oct 11, 2018

Exemplary pull request, @rmacklin! I will give this a closer look soon.

Thank you! 🍻

I have some additional questions and ideas that we could discuss to improve this.

I'm all ears :)

Great! Here goes:

  1. I'm not familiar with Blade (although I love the command bundle exec blade runner 😄), but it appears that's what was previously responsible for building the ActionCable javascript bundle. Now that this PR has added rollup to handle the build, is it correct to assume we can/should remove this:

    build:
    logical_paths:
    - action_cable.js
    path: lib/assets/compiled
    clean: true
    ?

  2. On a related note, it looks like Blade is currently used to run some javascript tests in https://github.com/rails/rails/tree/8df7ed3b88fc9f19446d7207a745a331893b81cd/actioncable/test/javascript . I ran bundle exec rake test:integration and visited http://0.0.0.0:9876/ and still got 34 assertions of 34 passed, 0 failed when using the rollup-compiled bundle. So it doesn't seem like a change is necessary, but I'm still wondering if we want to continue using Blade as the javascript test runner? Or do we want to move to a node-based test framework (e.g. mocha) now that were using a node-based build tool (rollup)?

  3. There are some circular dependencies in the ActionCable modules (they're present in the CoffeeScript version on master, too, but they're surfaced more visibly in this PR now that we're using ES2015 module syntax). In particular:

    • connection.js imports index.js to use ActionCable.log and ActionCable.WebSocket
    • connection_monitor.js imports index.js to use ActionCable.log

    These circular dependencies don't seem to break anything (though rollup emits a warning about them during compilation), but what are your thoughts about refactoring to avoid them? I think we could break the circle by adding separate adapters.js and logger.js modules.

    • startDebugging, stopDebugging and log would be moved to the logger.js module. If desired, we could keep these methods on the main ActionCable export by delegating to logger, but I think it's cleaner/simpler to skip that. It would be a small breaking API change, but could easily be patched in applications that didn't want to update their usages of these methods:
      import ActionCable from 'actioncable'; // stays the same between actioncable v5.x and v6
      
      // this patch would keep the remaining code working without modification
      Object.defineProperties(ActionCable, {
        startDebugging: {
          get() { return ActionCable.Logger.startDebugging; },
          set(value) { ActionCable.Logger.startDebugging = value; }
        },
        stopDebugging: {
          get() { return ActionCable.Logger.stopDebugging; },
          set(value) { ActionCable.Logger.stopDebugging = value; }
        },
        log: {
          get() { return ActionCable.Logger.log; },
          set(value) { ActionCable.Logger.log = value; }
        }
      });
      
      ActionCable.startDebugging();
      ActionCable.log('whatever');
    • Configuration of ActionCable.WebSocket and ActionCable.logger would be moved to the adapters.js module. Again, I think it would be okay to make this a small breaking API change, especially because I would assume the vast majority of ActionCable users do not override the default configuration. Users that do override the defaults and don't want to update their code could still patch the old API into their applications:
      Object.defineProperties(ActionCable, {
        WebSocket: {
          get() { return ActionCable.Adapters.WebSocket; },
          set(value) { ActionCable.Adapters.WebSocket = value; }
        },
        logger: {
          get() { return ActionCable.Adapters.logger; },
          set(value) { ActionCable.Adapters.logger = value; }
        }
      });

Okay, I believe that was everything I wanted to discuss. I think (2) and (3) could be addressed in follow-up PRs if we decide they are worth addressing. Let me know if my assumption is correct re: (1) and I can push a new commit to this PR. And please also let me know if you have any comments on the existing changes in this PR!

@javan

This comment has been minimized.

Copy link
Member

commented Oct 12, 2018

1 … Now that this PR has added rollup to handle the build, is it correct to assume we can/should remove this

Yeah, you can remove those lines from .blade.yml. Doing so means we're assuming the compiled app/assets/javascripts/action_cable.js will be present so we may want to add it to version control like Active Storage does.

2 …wondering if we want to continue using Blade as the javascript test runner?

Ultimately, no. I'd like to drop Blade entirely and switch to something like Karma. Let's leave that for a separate change though if the tests are running/passing now.

3 There are some circular dependencies in the ActionCable modules (they're present in the CoffeeScript version on master, too, but they're surfaced more visibly in this PR now that we're using ES2015 module syntax)

They're technically not circular in the CoffeeScript version since it doesn't have any true modules to require / import.

I think we could break the circle by adding separate adapters.js and logger.js modules. … If desired, we could keep these methods on the main ActionCable export by delegating to logger, but I think it's cleaner/simpler to skip that. It would be a small breaking API change, but could easily be patched in applications that didn't want to update their usages of these methods

I think you can preserve the current API while still adding separate modules and eliminating the circular dependencies. Something like:

// logger.js
export default {
  logger: window.console,
  enabled: false,

  log: (...messages) {
    if (this.enabled) {
      messages.push(Date.now())
      this.logger.log("[ActionCable]", ...messages)
    }
  }
}

// index.js
import logger from "./logger"

const ActionCable = {
  //

  set logger(value) {
    logger.logger = value
  }
  
  startDebugging() {
    logger.enabled = true
  },

  stopDebugging() {
    logger.enabled = false
  },

  log(...messages) {
    logger.log(...messages)
  }
}
@rmacklin

This comment has been minimized.

Copy link
Contributor Author

commented Oct 16, 2018

Ultimately, no. I'd like to drop Blade entirely and switch to something like Karma. Let's leave that for a separate change though if the tests are running/passing now.

Sounds good

Yeah, you can remove those lines from .blade.yml. Doing so means we're assuming the compiled app/assets/javascripts/action_cable.js will be present so we may want to add it to version control like Active Storage does.

👍 I added the compiled bundle to source control and removed that section of blade.yml.

Consequently, I removed the related assets:compile and assets:verify rake tasks. Now that the compiled bundle is in source control, verifying the bundle can happen before changes are merged, rather than at publish-time. We could potentially add those verifications to the test suite, but I haven't done that in this PR for a couple of reasons:

  • For one, I didn't see these verifications in ActiveStorage, which I suspect is because we are trusting that rollup will generate a proper UMD module when we specify format: "umd" in its config.
  • Additionally, if we plan to switch to something like Karma for the javascript tests, that might be a better time to write the test that verifies that the module can be required in node. (And we could take it further than only verifying that ActionCable can be required - we could also decide to test ActionCable's functionality in a node environment.)

In the meantime, we should be able to catch breakages to the module format during code review, since git will now track the diff of the compiled bundle. That said, let me know if you think we absolutely need to replace the assets:verify checks with something else as part of this PR.

@rmacklin rmacklin force-pushed the rmacklin:convert-actioncable-to-es2015 branch from 3872bcd to a3836df Oct 16, 2018

@rmacklin

This comment has been minimized.

Copy link
Contributor Author

commented Oct 16, 2018

There are some circular dependencies in the ActionCable modules (they're present in the CoffeeScript version on master, too, but they're surfaced more visibly in this PR now that we're using ES2015 module syntax).

They're technically not circular in the CoffeeScript version since it doesn't have any true modules to require / import.

Yeah that's true; I just meant that conceptually the structure of these files and their dependencies wasn't updated by the switch to ES2015 modules.

It's just that before there was an implicit dependency from ActionCable.Connection to ActionCable.log at runtime, whereas in the ES2015 module version, the dependencies are explicit via the static import statements, which leads to the warning about circular dependencies at compile time. (It's just a warning though, and it still compiles correctly. Anyway, it sounds like you're in favor of removing the circular dependencies, so I went ahead with that.)

I think you can preserve the current API while still adding separate modules and eliminating the circular dependencies.

Yeah, you definitely can preserve the API. That's what I meant by:

If desired, we could keep these methods on the main ActionCable export by delegating to logger, but I think it's cleaner/simpler to skip that.

The reason I think it's cleaner to skip that is because I think it's better for a package to provide one interface. The reason being: if my application only uses log via an export from the actioncable/logger module, I shouldn't even ship the code for the unused ActionCable.log alias to my end users. (If this were server code, it wouldn't really matter, but any additional javascript that we have to ship to the client comes at a cost paid by every end user.)

That said, I can understand the desire to avoid breaking backwards compatibility so that users can upgrade more easily. I just pushed some new changes that keep the API backwards compatible for the compiled bundle but avoid shipping the old API for users that consume the untranspiled source code.

That seems like the best of both worlds: upgrading won't break anything, but if you want to import from actioncable/src you'll need to update any usages of log and configuration of the WebSocket adapter (but if you aren't using log or changing the adapter, you wouldn't need to change anything). And if you use a module-aware/tree-shaking bundler, your application will not include any functions that you don't import, such as startDebugging and stopDebugging (so your bundle will be even smaller). With these changes, initial example bundle savings that I reported improve to 3.4K (30.4%).

@rmacklin rmacklin force-pushed the rmacklin:convert-actioncable-to-es2015 branch 2 times, most recently from 42136bf to ae122c3 Oct 18, 2018

@rmacklin

This comment has been minimized.

Copy link
Contributor Author

commented Oct 19, 2018

Per #34177 (comment), I have moved the module changes that are not related to the conversion from CoffeeScript into a separate PR: #34258

@rmacklin

This comment has been minimized.

Copy link
Contributor Author

commented Oct 19, 2018

There is one failed build, Ruby: 2.5.1 GEM=ap,ac (passed on ruby 2.4.4), but it died after only a few seconds:
https://travis-ci.org/rails/rails/jobs/443609899
I think I can close and reopen this PR but I think that will rerun everything. So @javan if you're already looking at this perhaps you can just rerun that single build. Otherwise I'll close and reopen.

@rmacklin rmacklin closed this Oct 19, 2018

@rmacklin rmacklin reopened this Oct 19, 2018

@rmacklin

This comment has been minimized.

Copy link
Contributor Author

commented Oct 19, 2018

CI is all good now!

@staugaard

This comment has been minimized.

Copy link
Contributor

commented Oct 24, 2018

Let's do this.

@javan

This comment has been minimized.

Copy link
Member

commented Oct 26, 2018

@rmacklin, sorry for the delay. We (Basecamp) have our company meetup this week up. I'll pick this back up next week.

@rmacklin

This comment has been minimized.

Copy link
Contributor Author

commented Oct 26, 2018

Thanks for the update! Looking forward to getting this wrapped up :)

@javan

This comment has been minimized.

Copy link
Member

commented Nov 1, 2018

Just left a couple more little comments. Once those are addressed can squash your commits, please? 1 to 3 meaningful commits is fine. After that we're good to merge.

@rmacklin

This comment has been minimized.

Copy link
Contributor Author

commented Nov 1, 2018

Addressed the two changes. Rebasing and squashing will take me some time. In the meantime, can you take a look at my comments on #34258 (comment) and let me know if rmacklin@832225e is the right interpretation of what you'd like to happen there? I'd like to continue working on that one after I rebase and squash this one.

@javan

This comment has been minimized.

Copy link
Member

commented Nov 2, 2018

One more thing… We need to include the new app/assets/javascripts/action_cable.js file in the published gem for Sprockets support. Currently, app/ isn't the s.files array. Here's activestorage.gemspec for comparison.

@rmacklin

This comment has been minimized.

Copy link
Contributor Author

commented Nov 2, 2018

One more thing… We need to include the new app/assets/javascripts/action_cable.js file in the published gem for Sprockets support. Currently, app/ isn't the s.files array. Here's activestorage.gemspec for comparison.

Added in rmacklin@85fa4fc

I'm ready to start squashing, but is there anything else?

I've also opened #34370 with what I believe is the approach you were describing in the #34177 (review) thread. It's simpler than the approach in #34258 because avoids introducing a new action_cable.js file at the cost of slightly modifying the API for setting adapters. I am fine with either approach, so if you like #34370 I can continue working there and close #34258, or vice versa.

@javan

This comment has been minimized.

Copy link
Member

commented Nov 2, 2018

Squash away! Apologies, but I haven't had tome to review your related PRs yet. I will once this one is squared away.

rmacklin added some commits Dec 11, 2017

Move actioncable javascript to app/javascript and change .coffee -> .js
- Rename action_cable/*.coffee -> *.js

- Move app/assets/javascripts/* -> app/javascript/*

- Rename action_cable.js.erb -> action_cable/index.js.erb

Renaming the extension to .js is in preparation for converting these
files from coffeescript to ES2015.

Moving the files to app/javascript and putting the entry point in
index.js.erb changes the structure of ActionCable's javascript to match
the structure of ActiveStorage's javascript.

(We are doing the file moving and renaming in a separate commit to
ensure that the git history of the files will be preserved - i.e. git
will track these as file renames rather than unrelated file
additions/deletions. In particular, git blame will still trace back to
the original authorship.)
Run decaffeinate on action_cable/*.js
Using [decaffeinate], we have converted these files from coffeescript
syntax to ES2015 syntax. Decaffeinate is very conservative in the
conversion process to ensure exact coffeescript semantics are preserved.
Most of the time, it's safe to clean up the code, and decaffeinate has
left suggestions regarding potential cleanups we can take. I'll tackle
those cleanups separately.

After running decaffeinate, I ran:
```
eslint --fix app/javascript
```
using the eslint configuration from ActiveStorage to automatically
correct lint violations in the decaffeinated output. This removed 189
extra semicolons and changed one instance of single quotes to double
quotes.

Note: decaffeinate and eslint can't parse ERB syntax. So I worked around
that by temporarily quoting the ERB:
```diff
 @ActionCable =
-  INTERNAL: <%= ActionCable::INTERNAL.to_json %>
+  INTERNAL: "<%= ActionCable::INTERNAL.to_json %>"
   WebSocket: window.WebSocket
   logger: window.console
```
and then removing those quotes after running decaffeinate and eslint.

[decaffeinate]: https://github.com/decaffeinate/decaffeinate
Refactor decaffeinate output to more natural/idiomatic javascript
- Remove unnecessary Array.from usages from subscriptions.js

  These were all Arrays before, so Array.from is a no-op

- Remove unnecessary IIFEs from subscriptions.js

- Manually decaffeinate sample ActionCable code in comments

  Here the coffeescript -> ES2015 conversion was done by hand rather than
  using decaffeinate, because these code samples were simple enough.

- Refactor ActionCable.Subscription to avoid initClass

- Refactor ActionCable.Subscription to use ES2015 default parameters

- Refactor ActionCable.ConnectionMonitor to avoid initClass

- Refactor ActionCable.ConnectionMonitor to use shorter variations of null checks

- Remove unnecessary code created because of implicit returns in ConnectionMonitor

  This removes the `return` statements that were returning the value of
  console.log and those from private methods whose return value was not
  being used.

- Refactor ActionCable.Connection to avoid initClass

- Refactor Connection#isProtocolSupported and #isState

  This addresses these three decaffeinate cleanup suggestions:
  - DS101: Remove unnecessary use of Array.from
  - DS104: Avoid inline assignments
  - DS204: Change includes calls to have a more natural evaluation order

  It also removes the use of Array.prototype.includes, which means we
  don't have to worry about providing a polyfill or requiring that end
  users provide one.

- Refactor ActionCable.Connection to use ES2015 default parameters

- Refactor ActionCable.Connection to use shorter variations of null checks

- Remove return statements that return the value of console.log() in ActionCable.Connection

- Simplify complex destructure assignment in connection.js

  decaffeinate had inserted
  ```
  adjustedLength = Math.max(protocols.length, 1)
  ```
  to be safe, but we know that there has to always be at least one
  protocol, so we don't have to worry about protocols.length being 0 here.

- Refactor Connection#getState

  The decaffeinate translation of this method was not very clear, so we've
  rewritten it to be more natural.

- Simplify destructure assignment in connection.js

- Remove unnecessary use of Array.from from action_cable.js.erb

- Refactor ActionCable#createConsumer and #getConfig

  This addresses these two decaffeinate cleanup suggestions:
  - DS104: Avoid inline assignments
  - DS207: Consider shorter variations of null checks

- Remove unnecessary code created because of implicit returns in action_cable.js.erb

  This removes the `return` statements that were returning the value of
  console.log and those from methods that just set and unset the
  `debugging` flag.

- Remove decaffeinate suggestion about avoiding top-level this

  In this case, the top-level `this` is intentional, so it's okay to
  ignore this suggestion.

- Remove decaffeinate suggestions about removing unnecessary returns

  I did remove some of the return statements in previous commits, where
  it seemed appropriate. However, the rest of these should probably remain
  because the return values have been exposed through the public API. If
  we want to break that contract, we can do so, but I think it should be
  done deliberately as part of a breaking-API change (separate from this
  coffeescript -> ES2015 conversion)

- Remove unused `unsupportedProtocol` variable from connection.js

  Leaving this would cause eslint to fail

- Refactor Subscriptions methods to avoid `for` ... `of` syntax

  Babel transpiles `for` ... `of` syntax to use `Symbol.iterator`, which
  would require a polyfill in applications that support older browsers.

  The `for` ... `of` syntax was produced by running `decaffeinate`, but in
  these instances a simpler `map` should be sufficient and avoid any
  `Symbol` issues.
Convert ActionCable javascript to ES2015 modules with modern build en…
…vironment

We've replaced the sprockets `//= require` directives with ES2015
imports. As a result, the ActionCable javascript can now be compiled
with rollup (like ActiveStorage already is).

- Rename action_cable/index.js.erb -> action_cable/index.js

- Add rake task to generate a javascript module of the ActionCable::INTERNAL ruby hash

  This will allow us to get rid of ERB from the actioncable javascript,
  since it is only used to interpolate ActionCable::INTERNAL.to_json.

- Import INTERNAL directly in ActionCable Connection module

  This is necessary to remove a load-order dependency conflict in the
  rollup-compiled build. Using ActionCable.INTERNAL would result in a
  runtime error:
  ```
  TypeError: Cannot read property 'INTERNAL' of undefined
  ```
  because ActionCable.INTERNAL is not set before the Connection module
  is executed.

  All other ActionCable.* references are executed inside of the body of a
  function, so there is no load-order dependency there.

- Add eslint and eslint-plugin-import devDependencies to actioncable

  These will be used to add a linting setup to actioncable like the one
  in activestorage.

- Add .eslintrc to actioncable

  This lint configuration was copied from activestorage

- Add lint script to actioncable

  This is the same as the lint script in activestorage

- Add babel-core, babel-plugin-external-helpers, and babel-preset-env devDependencies to actioncable

  These will be used to add ES2015 transpilation support to actioncable
  like we have in activestorage.

- Add .babelrc to actioncable

  This configuration was copied from activestorage

- Enable loose mode in ActionCable's babel config

  This generates a smaller bundle when compiled

- Add rollup devDependencies to actioncable

  These will be used to add a modern build pipeline to actioncable like
  the one in activestorage.

- Add rollup config to actioncable

  This is essentially the same as the rollup config from activestorage

- Add prebuild and build scripts to actioncable package

  These scripts were copied from activestorage

- Invoke code generation task as part of actioncable's prebuild script

  This will guarantee that the action_cable/internal.js module is
  available at build time (which is important, because two other modules
  now depend on it).

- Update actioncable package to reference the rollup-compiled files

  Now that we have a fully functional rollup pipeline in actioncable, we
  can use the compiled output in our npm package.

- Remove build section from ActionCable blade config

  Now that rollup is responsible for building ActionCable, we can remove
  that responsibility from Blade.

- Remove assets:compile and assets:verify tasks from ActionCable

  Now that we've added a compiled ActionCable bundle to version control,
  we don't need to compile and verify it at publish-time.

  (We're following the pattern set in ActiveStorage.)

- Include compiled ActionCable javascript bundle in published gem

  This is necessary to maintain support for depending on the ActionCable
  javascript through the Sprockets asset pipeline.

- Add compiled ActionCable bundle to version control

  This mirrors what we do in ActiveStorage, and allows ActionCable to
  continue to be consumed via the sprockets-based asset pipeline when
  using a git source instead of a published version of the gem.

@rmacklin rmacklin force-pushed the rmacklin:convert-actioncable-to-es2015 branch from 85fa4fc to c96139a Nov 2, 2018

@rmacklin

This comment has been minimized.

Copy link
Contributor Author

commented Nov 2, 2018

Updated with the squashed version.

The first commit is only file moving and renaming. I kept that as a separate commit to allow the git history of the files to be preserved (i.e. ensure git will track these as file renames rather than unrelated file additions/deletions). In particular, you can easily trace the git blame back to the original authorship:

file_history_intact

The remaining meaningful commits are:

  • Run automated conversions on the coffeescript files
  • Manually clean up converted code (I wanted to keep a clear separation between the conversion done by machine and the subsequent cleanup done by hand)
  • Convert to ES2015 modules and modify the build environment to support this (I also didn't want to muddy the waters by mixing the decaffeinate cleanup with the conversion of IIFEs and sprockets //= requires into ES2015 modules)
@rmacklin

This comment has been minimized.

Copy link
Contributor Author

commented Nov 2, 2018

CI isn't finished yet, but the CI job for GEM=aj:integration for Ruby 2.4.4 failed during apt-get install:
https://travis-ci.org/rails/rails/jobs/449908043

That seems like an intermittent failure, so I'll close and reopen to retrigger CI.

@rmacklin rmacklin closed this Nov 2, 2018

@rmacklin rmacklin reopened this Nov 2, 2018

@rmacklin rmacklin closed this Nov 2, 2018

@rmacklin rmacklin reopened this Nov 2, 2018

@rmacklin

This comment has been minimized.

Copy link
Contributor Author

commented Nov 2, 2018

CI is back in the green 🎉

@javan

This comment has been minimized.

Copy link
Member

commented Nov 2, 2018

Looks good!

Quick question: Have you tried using the compiled action_cable.js in a working application (that actually makes a WebSocket connection)?

@rmacklin

This comment has been minimized.

Copy link
Contributor Author

commented Nov 2, 2018

I created the use-es2015-actioncable branch in this repository: https://github.com/rmacklin/anycable_demo

(It's a fork of https://github.com/anycable/anycable_demo, but the repository supports regular ActionCable in addition to AnyCable)

Running:

git clone git@github.com:rmacklin/anycable_demo.git
cd anycable_demo
git checkout use-es2015-actioncable
bin/setup
bin/rails server

and then visiting localhost:3000 demonstrates a working application using this PR's version of action_cable.js to make a real WebSocket connection.

@javan javan merged commit b858c2c into rails:master Nov 2, 2018

2 checks passed

codeclimate All good!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@javan

This comment has been minimized.

Copy link
Member

commented Nov 2, 2018

Thank you!

@rmacklin rmacklin deleted the rmacklin:convert-actioncable-to-es2015 branch Nov 2, 2018

@rmacklin

This comment has been minimized.

Copy link
Contributor Author

commented Nov 2, 2018

Thanks for your help!

@audiolion

This comment has been minimized.

Copy link
Contributor

commented Dec 5, 2018

@rmacklin @javan 🎉 !!! so excited!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.