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

Remove circular dependency warnings in ActionCable javascript and publish source modules with fine-grained exports #34370

Conversation

@rmacklin
Copy link
Contributor

commented Nov 2, 2018

This is an alternative approach to #34258. In this version, there is no separate action_cable.js file. As a result, there are two minor breaking API changes.

Summary

In #34177, we've converted the ActionCable javascript to ES2015 (in that PR we avoided refactoring the existing structure, which has some circular dependency warnings). This PR builds on top of that work. In this PR we:

  1. Reorganize the module imports to remove the circular dependency warnings
  2. Update the ActionCable npm package to ship untranspiled ES2015 source code in addition to the heavier compiled bundle, and we offer fine-grained exports to provide optimal tree-shaking
    • 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

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.

Note: This produces two small breaking API changes:

  • The ActionCable.WebSocket getter/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:

    -    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/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:

    -    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:

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. That alternative approach is in #34258.)

This API change is very minor - it should be easy for applications to add the adapters. prefix in their assignments or to temporarily 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.

In an example repository, the bundle shrinks by 3.2K (30%) when you simply change the actioncable import to point to the untranspiled tree-shaking-friendly source modules:

resolve: {
  alias: {
    actioncable: 'actioncable/src'
  }
}
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', {
@rails-bot

This comment has been minimized.

Copy link

commented Nov 2, 2018

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.

@rails-bot rails-bot bot added the actioncable label Nov 2, 2018

@rmacklin rmacklin force-pushed the rmacklin:remove-circular-dependency-warnings-in-actioncable-and-publish-source-modules branch 2 times, most recently from 1e40b02 to 1ee299e Nov 2, 2018

@rmacklin rmacklin closed this Nov 2, 2018

@rmacklin rmacklin reopened this Nov 2, 2018

@eileencodes

This comment has been minimized.

Copy link
Member

commented Nov 3, 2018

r? @javan

Re-assigning because JavaScript & Action Cable aren't my expertise 😄

@eileencodes eileencodes assigned javan and unassigned eileencodes Nov 3, 2018

@rmacklin rmacklin force-pushed the rmacklin:remove-circular-dependency-warnings-in-actioncable-and-publish-source-modules branch 2 times, most recently from 2b07bcc to 63dbb6a Nov 17, 2018

@rmacklin

This comment has been minimized.

Copy link
Contributor Author

commented Nov 17, 2018

The PR had a merge conflict in actioncable/app/assets/javascripts/action_cable.js. I fixed it but I rebased off of #34475 (not master) so that the generated bundle would be correct.

Edit: #34475 was merged, so this is now rebased off master.

@rmacklin

This comment has been minimized.

Copy link
Contributor Author

commented Nov 17, 2018

@javan I'd still love to ship the tree-shakeable untranspiled source with the ActionCable node module. Do you have an opinion on which approach you like more between this and the approach in #34258? I'm good either way, so let me know which direction I should work on 😄

@rmacklin rmacklin force-pushed the rmacklin:remove-circular-dependency-warnings-in-actioncable-and-publish-source-modules branch from 63dbb6a to a9a787a Nov 18, 2018

@javan

This comment has been minimized.

Copy link
Member

commented Dec 1, 2018

Looking good! Mind rebasing and sorting out any conflicts? Once that and #34370 (comment) are done, I'll merge.

rmacklin added some commits Oct 19, 2018

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

@rmacklin rmacklin force-pushed the rmacklin:remove-circular-dependency-warnings-in-actioncable-and-publish-source-modules branch from cadf99b to c79d8ec Dec 1, 2018

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.

@rmacklin rmacklin force-pushed the rmacklin:remove-circular-dependency-warnings-in-actioncable-and-publish-source-modules branch 2 times, most recently from e3d8e95 to 2e8fb53 Dec 1, 2018

@rmacklin

This comment has been minimized.

Copy link
Contributor Author

commented Dec 1, 2018

Looking good! Mind rebasing and sorting out any conflicts? Once that and #34370 (comment) are done, I'll merge.

Done!

@rmacklin

This comment has been minimized.

Copy link
Contributor Author

commented Dec 1, 2018

Hmm... There is an error running the tests on CI which does not happen for me locally:
https://travis-ci.org/rails/rails/jobs/462234456

I'm trying to look into it but let me know if you can reproduce locally

@javan

This comment has been minimized.

Copy link
Member

commented Dec 1, 2018

I get the same error locally:

$ node -v
v10.4.1

$ yarn test
yarn run v1.10.1
$ bundle exec rake assets:codegen && rollup --config rollup.config.test.js

test/javascript/src/test.js → test/javascript/compiled/test.js...
created test/javascript/compiled/test.js in 1.3s
$ karma start
internal/modules/cjs/loader.js:596
    throw err;
    ^

Error: Cannot find module 'flatted/cjs'
    at Function.Module._resolveFilename (internal/modules/cjs/loader.js:594:15)
    at Function.Module._load (internal/modules/cjs/loader.js:520:25)
    at Module.require (internal/modules/cjs/loader.js:650:17)
    at require (internal/modules/cjs/helpers.js:20:18)
    at Object.<anonymous> (/Users/javan/Work/javan/rails/node_modules/karma/lib/utils/json-utils.js:2:21)
    at Module._compile (internal/modules/cjs/loader.js:702:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:713:10)
    at Module.load (internal/modules/cjs/loader.js:612:32)
    at tryModuleLoad (internal/modules/cjs/loader.js:551:12)
    at Function.Module._load (internal/modules/cjs/loader.js:543:3)
    at Module.require (internal/modules/cjs/loader.js:650:17)
    at require (internal/modules/cjs/helpers.js:20:18)
    at Object.<anonymous> (/Users/javan/Work/javan/rails/node_modules/karma/lib/server.js:14:19)
    at Module._compile (internal/modules/cjs/loader.js:702:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:713:10)
    at Module.load (internal/modules/cjs/loader.js:612:32)
@rmacklin

This comment has been minimized.

Copy link
Contributor Author

commented Dec 1, 2018

Ok, thanks. After wiping my node_modules folder and reinstalling I can reproduce.

I also am noticing that when I install a new package (e.g. yarn add --dev left-pad) in either rails/actioncable or rails/activestorage, it doesn't update the yarn.lock file contained in that directory. So my guess these local yarn.lock files no longer matter, possibly because the repository now has top-level configuration for yarn workspaces:

rails/.yarnrc

Lines 1 to 2 in a429b29

workspaces-experimental true
--add.prefer-offline true

rails/package.json

Lines 3 to 10 in a429b29

"workspaces": [
"actioncable",
"activestorage",
"actionview",
"tmp/templates/app_template",
"railties/test/fixtures/tmp/bukkits/**/test/dummy",
"railties/test/fixtures/tmp/bukkits/**/spec/dummy"
],

(added in #33079), and previously my node_modules folder had all the packages installed without that change.

I haven't used yarn workspaces before, but I'll look into this now that I have a local repro. Separately, are the yarn.lock files in the actioncable and activestorage directories obsolete now?

@javan

This comment has been minimized.

Copy link
Member

commented Dec 1, 2018

Seems to stem from the Yarn Workspaces configured in the root package.json:

rails/package.json

Lines 3 to 10 in a429b29

"workspaces": [
"actioncable",
"activestorage",
"actionview",
"tmp/templates/app_template",
"railties/test/fixtures/tmp/bukkits/**/test/dummy",
"railties/test/fixtures/tmp/bukkits/**/spec/dummy"
],

Running yarn install from the root fixed it for me:

$ yarn install
$ cd actioncable
$ yarn test
yarn run v1.10.1
$ bundle exec rake assets:codegen && rollup --config rollup.config.test.js

test/javascript/src/test.js → test/javascript/compiled/test.js...
created test/javascript/compiled/test.js in 1.3s
$ karma start
01 12 2018 14:16:37.240:INFO [karma-server]: Karma v3.1.3 server started at http://0.0.0.0:9876/
01 12 2018 14:16:37.243:INFO [launcher]: Launching browsers ChromeHeadless with concurrency unlimited
01 12 2018 14:16:37.248:INFO [launcher]: Starting browser ChromeHeadless
01 12 2018 14:16:37.618:INFO [HeadlessChrome 70.0.3538 (Mac OS X 10.14.1)]: Connected on socket pkCxK_mMIXun8KNCAAAA with id 23595082
HeadlessChrome 70.0.3538 (Mac OS X 10.14.1): Executed 13 of 13 SUCCESS (0.087 secs / 0.134 secs)
TOTAL: 13 SUCCESS
✨  Done in 4.19s.
@javan

This comment has been minimized.

Copy link
Member

commented Dec 1, 2018

jinx

@javan

This comment has been minimized.

Copy link
Member

commented Dec 1, 2018

Gosh, this whole thing is a mess:

rails/.travis.yml

Lines 40 to 45 in a429b29

- "[[ $GEM != 'ac:integration' ]] || (cd actioncable && yarn install)"
- "[[ $GEM != 'av:ujs' ]] || nvm install node"
- "[[ $GEM != 'av:ujs' ]] || node --version"
- "[[ $GEM != 'av:ujs' ]] || (cd actionview && npm install)"
- "[[ $GEM != 'railties' ]] || (curl -o- -L https://yarnpkg.com/install.sh | bash)"
- "[[ $GEM != 'railties' ]] || export PATH=$HOME/.yarn/bin:$PATH"

But I think we need to configure CI to run yarn install from the root rather than actioncable/

@rmacklin

This comment has been minimized.

Copy link
Contributor Author

commented Dec 1, 2018

Running yarn install from the root fixed it for me:

This didn't work for me initially. I had a local yarn.lock in the root, which I didn't notice because it's in the root .gitignore:

/yarn.lock

I must have run yarn install at the root once before and it generated that. After removing the root yarn.lock and then running yarn install at the root, I was able to get it passing again. I'll change the line in the travis config to run yarn install at the top level.

I think I'm going to open a separate issue about the yarn lockfiles. It seems to me that the yarn.lock files in actioncable and activestorage should be removed and the top-level yarn.lock should be checked in rather than gitignored. Otherwise people may run into the same issue as me with a stale yarn.lock that they don't realize, which prevents them from correctly installing new packages.

Edit: Issue opened

@rmacklin

This comment has been minimized.

Copy link
Contributor Author

commented Dec 1, 2018

🎉 ActionCable tests passed on CI!
https://travis-ci.org/rails/rails/jobs/462264726

rmacklin added some commits Oct 19, 2018

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
c0368ad
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 Actio…
…nCable builds

This is necessary now that the repository is using Yarn Workspaces

@rmacklin rmacklin force-pushed the rmacklin:remove-circular-dependency-warnings-in-actioncable-and-publish-source-modules branch from c0d3803 to a1bcd06 Dec 1, 2018

@javan javan merged commit aa1ba9c into rails:master Dec 1, 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 Dec 1, 2018

Thank you once again!

Any interest in taking on actionview next? 😬

@rmacklin

This comment has been minimized.

Copy link
Contributor Author

commented Dec 1, 2018

Thank you once again!

🍻 Thanks for your help! I really like where this ended up, especially after properly understanding what you were suggesting with the logger API.

Any interest in taking on actionview next? 😬

I'm not using the actionview javascript in my current project, so I'm not familiar with it. (I have used jquery_ujs in past projects, but it's been a while. Is it correct that the new package is mostly the same but implemented without jQuery?) So, I don't want to make any promises on updating it, but I will try to find some time to at least take a look.

@rmacklin rmacklin deleted the rmacklin:remove-circular-dependency-warnings-in-actioncable-and-publish-source-modules branch Dec 1, 2018

javan added a commit that referenced this pull request Aug 15, 2019

Ensure @rails/actioncable package contains complete source
Action Cable's JavaScript library can optionally be imported as an ES6 module via `import { … } from "@rails/actioncable/src"`, but that module is broken in most of the releases published on npm:

```
ERROR in ./node_modules/@rails/actioncable/src/connection.js
Module not found: Error: Can't resolve './internal' in './node_modules/@rails/actioncable/src'
 @ ./node_modules/@rails/actioncable/src/connection.js
 @ ./node_modules/@rails/actioncable/src/index.js
```

Because `internal.js` was gitignored, it would only be included if the publisher happened to have it generated locally. Committing it to version control ensures that won't happen, and gives us better visibility into changes over time.

References:
- #34370
- c0368ad
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.