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 library from CoffeeScript to JavaScript #33380
Conversation
Thanks for the pull request, and welcome! The Rails team is excited to review your changes, and you should hear from @eileencodes (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. |
If we would do this, and only if, it would be to use ES6 like Active Storage do. Changing CoffeScript with ES5 is not buying us anything. |
I agree with @rafaelfranca and suggest using the exact rollup+babel+eslint rig from Active Storage: rails/activestorage/package.json Lines 35 to 38 in e13e16f
|
Great! @varyonic are you going to work on it? |
I'm not familiar with rollup but I'll give it a try. |
|
@javan Does Blade need to be replaced with Karma or similar here? If so any examples I should look at for guidance? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great start!
Generally, the code feels like a direct translation of the CoffeeScript and not necessarily how you'd write it from scratch in ES6.
Does Blade need to be replaced with Karma or similar here?
Yes, please. :) We should remove Blade / Sprockets dependencies entirely. I'll try to find you a good example to reference.
let now = undefined; | ||
let secondsSince = undefined; | ||
let clamp = undefined; | ||
class ConnectionMonitor { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the compiled actioncable.js
file, right? There shouldn't be any ES6 in here…
// ,unsupportedProtocol = protocols[adjustedLength - 1] | ||
|
||
export class Connection { | ||
static initClass() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not quite following these static initClass()
methods that get executed immediately. I would use static properties instead:
export class Connection {
static reopenDelay = 500
// …
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this valid ES6? The code feels like a direct translation because it is. I was hoping to have working tests before refactoring.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not yet… You can use the transform-class-properties Babel plugin. Or, just assign those properties after the class definition:
export class Connection {
// …
}
Connection.reopenDelay = 500
if (this.debugging) { | ||
messages.push(Date.now()) | ||
this.logger = window.console | ||
return this.logger.log("[ActionCable]", ...Array.from(messages)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't need Array.from()
here: this.logger.log("[ActionCable]", ...messages)
|
||
let now = undefined | ||
let secondsSince = undefined | ||
let clamp = undefined |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need to start these out undefined
and then set them later (immediately, basically).
|
||
export function stopDebugging() { | ||
return this.debugging = null | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The debugging
flag should only be set on the top level object, but these will set it on whatever happens to be this
. I'd avoid using this
in helpers, generally.
|
||
createConsumer(url) { | ||
if (url == null) { let left | ||
url = (left = this.getConfig("url")) != null ? left : this.INTERNAL.default_mount_path } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is INTERNAL
defined anywhere? Will need some kind of setup to replace:
@ActionCable = | |
INTERNAL: <%= ActionCable::INTERNAL.to_json %> |
reject(identifier) { | ||
return (() => { | ||
const result = [] | ||
for (let subscription of Array.from(this.findAll(identifier))) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there are browsers that support WebSocket
, but not Array.from()
so we should avoid using it if possible.
Move javascript out of app/assets
Use ES6 modules.
karma.conf for Rollup/RequireJS plus boilerplate test page to load and run unit tests.
I added and configured karma-qunit but the tests are erroring "Uncaught Error: pushFailure() assertion outside test context" I think that's something to do with mock-socket not being setup correctly but I don't understand how to troubleshoot it. |
For the Karma setup, I think it'd be more straightforward to compile a single .js file first and then start the runner. That way you don't need to muck around with requirejs and stash a list of a files globally. See https://github.com/josh/selector-observer for a good example of this. |
Two thoughts:
|
See #34177 |
In response to #33331