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

Commits on Nov 2, 2018

  1. 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.)
    rmacklin committed Nov 2, 2018
    Configuration menu
    Copy the full SHA
    7b0b372 View commit details
    Browse the repository at this point in the history
  2. 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
    rmacklin committed Nov 2, 2018
    Configuration menu
    Copy the full SHA
    403c001 View commit details
    Browse the repository at this point in the history
  3. 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.
    rmacklin committed Nov 2, 2018
    Configuration menu
    Copy the full SHA
    0eb6b86 View commit details
    Browse the repository at this point in the history
  4. 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 committed Nov 2, 2018
    Configuration menu
    Copy the full SHA
    c96139a View commit details
    Browse the repository at this point in the history