1
0
Fork 0
mirror of https://github.com/rails/rails.git synced 2022-11-09 12:12:34 -05:00
Commit graph

9 commits

Author SHA1 Message Date
Richard Macklin
739f88e52e Simplify this.isActive() && this.webSocket into this.isActive()
in Connection#close. We can do this because `isActive()` can only
return `true` if `this.webSocket` is truthy. (We can't have an active
connection without having instantiated a WebSocket. This is confirmed
in the code: Connection#isActive calls Connection#isState which calls
Connection#getState, which checks if `this.webSocket` is truthy and
returns `null` otherwise.)
2019-01-14 11:52:46 -08:00
Richard Macklin
6320916513 Simplify ActionCable.getConfig, Connection#getProtocol, and Connection#close
by relying on the implicit undefined return value
2019-01-14 11:35:04 -08:00
Mick Staugaard
58dbc1c2ed Stop trying to reconnect on unauthorized cable connections 2018-12-05 10:40:29 -08:00
Richard Macklin
2bb4fdef5e Replace reference to WebSocket global with ActionCable.adapters.WebSocket
The WebSocket dependency of ActionCable.Connection was made configurable
in 66901c1849

However, the reference here in Connection#getState was not updated to
use the configurable property. This change remedies that and adds a test
to verify it. Additionally, it backfills a test to ensure that
Connection#open uses the configurable property.
2018-12-01 14:49:43 -08:00
rmacklin
aa1ba9cb24 Remove circular dependency warnings in ActionCable javascript and publish source modules with fine-grained exports (#34370)
* Replace several ActionCable.* references with finer-grained imports

This reduces the number of circular dependencies among the module
imports from 4:

```
(!) Circular dependency: app/javascript/action_cable/index.js -> app/javascript/action_cable/connection.js -> app/javascript/action_cable/index.js
(!) Circular dependency: app/javascript/action_cable/index.js -> app/javascript/action_cable/connection_monitor.js -> app/javascript/action_cable/index.js
(!) Circular dependency: app/javascript/action_cable/index.js -> app/javascript/action_cable/consumer.js -> app/javascript/action_cable/index.js
(!) Circular dependency: app/javascript/action_cable/index.js -> app/javascript/action_cable/subscriptions.js -> app/javascript/action_cable/index.js
```

to 2:

```
(!) Circular dependency: app/javascript/action_cable/index.js -> app/javascript/action_cable/connection.js -> app/javascript/action_cable/index.js
(!) Circular dependency: app/javascript/action_cable/index.js -> app/javascript/action_cable/connection.js -> app/javascript/action_cable/connection_monitor.js -> app/javascript/action_cable/index.js
```

* Remove tests that only test javascript object property assignment

These tests really only assert that you can assign a property to
the ActionCable global object. That's true for pretty much any object
in javascript (it would only be false if the object has been frozen, or
has explicitly set some properties to be nonconfigurable).

* Refactor ActionCable to provide individual named exports

By providing individual named exports rather than a default export which
is an object with all of those properties, we enable applications to
only import the functions they need: any unused functions will be
removed via tree shaking.

Additionally, this restructuring removes the remaining circular
dependencies by extracting the separate adapters and logger modules, so
there are now no warnings when compiling the ActionCable bundle.

Note: This produces two small breaking API changes:

- The `ActionCable.WebSocket` getter and setter would be moved to
  `ActionCable.adapters.WebSocket`. If a user is currently configuring
  this, when upgrading they'd need to either add a delegated
  getter/setter themselves, or change it like this:
   ```diff
   -    ActionCable.WebSocket = MyWebSocket
   +    ActionCable.adapters.WebSocket = MyWebSocket
    ```
   Applications which don't change the WebSocket adapter would not need
   any changes for this when upgrading.

- Similarly, the `ActionCable.logger` getter and setter would be moved
  to `ActionCable.adapters.logger`. If a user is currently configuring
  this, when upgrading they'd need to either add a delegated
  getter/setter themselves, or change it like this:
   ```diff
   -    ActionCable.logger = myLogger
   +    ActionCable.adapters.logger = myLogger
    ```
   Applications which don't change the logger would not need any changes
   for this when upgrading.

These two aspects of the public API have to change because there's no
way to export a property setter for `WebSocket` (or `logger`) such that
this:
```js
import ActionCable from "actioncable"

ActionCable.WebSocket = MyWebSocket
```
would actually update `adapters.WebSocket`. (We can only offer that if
we have two separate source files like if `index.js` uses
`import * as ActionCable from "./action_cable" and then exports a
wrapper which has delegated getters and setters for those properties.)

This API change is very minor - it should be easy for applications to
add the `adapters.` prefix in their assignments or to patch in delegated
setters. And especially because most applications in the wild are not
ever changing the default value of `ActionCable.WebSocket` or
`ActionCable.logger` (because the default values are perfect), this API
breakage is worth the tree-shaking benefits we gain.

* Include source code in published actioncable npm package

This allows actioncable users to ship smaller javascript bundles to
visitors using modern browsers, as demonstrated in this repository:
https://github.com/rmacklin/actioncable-es2015-build-example

In that example, the bundle shrinks by 2.8K (25.2%) when you simply
change the actioncable import to point to the untranspiled src.

If you go a step further, like this:
```
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 * as ActionCable from 'actioncable';

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

 cable.subscriptions.create('AppearanceChannel', {
```

then the bundle shrinks by 3.6K (31.7%)!

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).

Note: This is the same enhancement that we made to activestorage in
c0368ad090

* Remove unused commonjs & resolve plugins from ActionCable rollup config

These were added when we copied the rollup config from ActiveStorage,
but ActionCable does not have any commonjs dependencies (it doesn't have
any external dependencies at all), so these plugins are unnecessary here

* Change ActionCable.startDebugging() -> ActionCable.logger.enabled=true

and ActionCable.stopDebugging() -> ActionCable.logger.enabled=false

This API is simpler and more clearly describes what it does

* Change Travis configuration to run yarn install at the root for ActionCable builds

This is necessary now that the repository is using Yarn Workspaces
2018-12-01 16:25:02 -05:00
Richard Macklin
c96139af71 Convert ActionCable javascript to ES2015 modules with modern build environment
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.
2018-11-02 08:41:05 -07:00
Richard Macklin
0eb6b86e96 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.
2018-11-02 08:40:59 -07:00
Richard Macklin
403c001c56 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
2018-11-02 08:40:35 -07:00
Richard Macklin
7b0b37240a 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.)
2018-11-02 08:40:10 -07:00
Renamed from actioncable/app/assets/javascripts/action_cable/connection.coffee (Browse further)