| Commit message (Collapse) | Author | Age | Files | Lines |
|
|
|
|
|
|
|
|
|
| |
Currently we sometimes find a redundant begin block in code review
(e.g. https://github.com/rails/rails/pull/33604#discussion_r209784205).
I'd like to enable `Style/RedundantBegin` cop to avoid that, since
rescue/else/ensure are allowed inside do/end blocks in Ruby 2.5
(https://bugs.ruby-lang.org/issues/12906), so we'd probably meets with
that situation than before.
|
|
|
|
| |
[timthez, Ilia Kasianenko]
|
| |
|
|
|
|
|
|
|
|
|
|
| |
The WebSocket dependency of ActionCable.Connection was made configurable
in 66901c1849efae74c8a58fe0cb36afd487c067cc
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.
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
| |
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
c0368ad090b79c19300a4aa133bb188b2d9ab611
* 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
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
| |
Karma and Rollup (#34440)
* Rename .coffee files in ActionCable test suite in prep for decaffeination
* Decaffeinate ActionCable tests
* Replace Blade with Karma and Rollup to run ActionCable JS tests
- Add karma and qunit devDependencies
- Add test script to ActionCable package
- Use rollup to bundle ActionCable tests
- Use karma as the ActionCable JS test runner
* Replace vendored mock-socket with package devDependency in ActionCable
* Move ActionCable yarn install to TravisCI before_install config
* Clean up decaffeinated ActionCable tests to use consistent formatting
|
|
|
|
| |
Fixes some typos.
|
|
|
|
|
|
|
|
|
|
|
| |
ActionCable::Channel::TestCase provides an ability
to unit-test channel classes.
There are several reasons to write unit/functional cable tests:
- Access control (who has access to the channel? who can perform action and with which argument?
- Frontend-less applications have no system tests at all–and we still need a way to test channels logic.
See also #27191
|
| |
|
|
|
|
| |
Follow up of #33798.
|
| |
|
|
|
|
| |
Test `assert_no_broadcasts` failure
|
|
|
|
| |
See `git grep "= Logger.new(nil)"`
|
| |
|
| |
|
|
|
|
|
|
| |
This FIXME belongs to a code example that was imported from the
internet. As we aren't going to do anything about it, I prefer to remove
it so it stops from appearing on searches.
|
|
|
|
|
|
|
| |
We sometimes ask "✂️ extra blank lines" to a contributor in reviews like
https://github.com/rails/rails/pull/33337#discussion_r201509738.
It is preferable to deal automatically without depending on manpower.
|
|
|
|
| |
introduced in a0ea528b61.
|
|\
| |
| | |
Refactor actioncable's tests
|
| |
| |
| |
| |
| |
| |
| |
| |
| | |
`ActionCable::TestCase`
Remove all `include ActiveSupport::Testing::MethodCallAssertions`
in actioncable's tests since we can do it only in `ActionCable::TestCase`
in order to prevent code duplication.
We use the same approach for other modules of Rails.
|
| |
| |
| |
| |
| |
| | |
We have defined `ActionCable::TestCase` in `actioncable/test/test_helper.rb`
that we can use in order to prevent code duplication and build common
interface for actioncable's test.
|
|\ \
| |/
|/|
| | |
Action Cable owns database connection, not Active Record
|
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| | |
Before this commit, the database connection used in Action Cable's
PostgreSQL adapter was "owned" by `ActiveRecord::Base.connection_pool`.
This meant that if, for example, `#clear_reloadable_connections!` was called on the pool, Active
Record would "steal" the database connection from Action Cable, and
would cause all sorts of issues. This became evident during file
reloads; despite Action Cable trying its hardest to return its borrowed
database connection to Active Record via `@pubsub.shutdown`, Active Record calls
`#clear_reloadable_connections!` on the connection pool, and due to the order of callbacks, Active
Record's callback was being executed first. This meant that if you tried
to rerender a view after a file was reloaded, you would have to wait
through Active Record's timeout and such.
Now, Action Cable takes direct ownership of the database connection it
uses. It removes the connection from the pool to avoid the situation
described above. Action Cable also makes sure to call `#disconnect!` on
the connection when appropriate, to match the previous behavior of
Active Record.
[ Jon Moss & Matthew Draper]
|
| |
| |
| |
| | |
Q.E.D.
|
| | |
|
| | |
|
| |
| |
| |
| |
| | |
Pull Request #32727 changed "mocha expects" in favor of `MethodCallAssertions`.
This commit fixes assertion that became less strict after the PR.
|
| | |
|
| | |
|
| | |
|
| | |
|
| |
| |
| |
| |
| | |
This autocorrects the violations after adding a custom cop in
3305c78dcd.
|
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| | |
Ruby 2.6.0 warns about this.
``` ruby -v
ruby 2.6.0dev (2018-04-04 trunk 63085) [x86_64-linux]
```
Before, see:
https://travis-ci.org/rails/rails/jobs/365740163#L1262-L1264
https://travis-ci.org/rails/rails/jobs/365944863#L2121-L2174
|
| |
| |
| |
| |
| | |
This has been possible since Mocha v1.0 and makes it clear that we want
Mocha to integrate with Minitest, not Test::Unit.
|
| | |
|
| | |
|
| | |
|
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| | |
I often face the following error when running test/connection/client_socket_test.rb:
```
$ bin/test test/connection/client_socket_test.rb:38
Run options: --seed 44035
# Running:
F
Failure:
ActionCable::Connection::ClientSocketTest#test_delegate_socket_errors_to_on_error_handler
[/app/actioncable/test/connection/client_socket_test.rb:47]:
--- expected
+++ actual
@@ -1 +1 @@
-["foo"]
+["Broken pipe", "Broken pipe", "Broken pipe", "foo"]
```
It can be reproduced easily by applying the following patch:
```
diff --git a/actioncable/test/connection/client_socket_test.rb b/actioncable/test/connection/client_socket_test.rb
index 2051216010..6bb9f13ea7 100644
--- a/actioncable/test/connection/client_socket_test.rb
+++ b/actioncable/test/connection/client_socket_test.rb
@@ -34,7 +34,8 @@ def on_error(message)
@server.config.allowed_request_origins = %w( http://rubyonrails.com )
end
- test "delegate socket errors to on_error handler" do
+ 1000.times do |i|
+ test "delegate socket errors to on_error handler #{i}" do
run_in_eventmachine do
connection = open_connection
@@ -47,6 +48,7 @@ def on_error(message)
assert_equal %w[ foo ], connection.errors
end
end
+ end
test "closes hijacked i/o socket at shutdown" do
run_in_eventmachine do
```
The cause is writing io from different thread at the same time.
`connection.process` sends handshake message from [StreamEventLoop's thread][] whereas
`connection.handle_open` sends welcome message from current thread.
[StreamEventLoop's thread]: https://github.com/rails/rails/blob/067fc779c4560fff4812614a2f78f9248f3e55f8/actioncable/lib/action_cable/connection/stream_event_loop.rb#L75
|
| |
| |
| |
| |
| |
| |
| |
| | |
Simplify our dev testing and CI story since we're also testing against
Redis for the Active Support cache store.
Directly test whether db, host, password, etc are passed through as
config instead of spinning up a Redis server with a password set on it.
|
| | |
|
| |
| |
| |
| | |
And enable `context_dependent` of Style/BracesAroundHashParameters cop.
|
| | |
|
| | |
|
| |
| |
| |
| |
| | |
(cherry picked from commit e2093c1f678175bde7c37c848686d979427346e1)
(cherry picked from commit d7dbe48273bd9e0adb1de5b52e3cdaeb4a65630b)
|
| | |
|
| |
| |
| |
| | |
It is covered by following assertion.
|
| |
| |
| |
| |
| | |
This reverts commit 3420a14590c0e6915d8b6c242887f74adb4120f9, reversing
changes made to afb66a5a598ce4ac74ad84b125a5abf046dcf5aa.
|
| | |
|
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| | |
- Adds RedisAdapterTest::AlternateConfiguration to account
for a relatively common alternative setup, as it’s used
as the first example in the
[Redis rubygem](https://github.com/redis/redis-rb#getting-started)
- Supplies original RedisAdapterTest with more complete
redis:// url format by adding a ‘userinfo’ (blank user),
so that it resembles the alternate configuration
- Supplies original EventedRedisAdapterTest with more complete
redis:// url as well
- Adds before_script to start redis-server with password as a daemon
and with explicit defaults copied from the default redis.conf
(Instead of using Travis' default init/upstart scripts for `redis` service)
|
| | |
|