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

Enforce that actioncable compiled JS bundle is in sync with source code #37485

Conversation

rmacklin
Copy link
Contributor

Summary

Addresses #34473 (comment)

We have run into issues in the past where the actioncable compiled javascript bundle got out of sync with the source code. For example, in 30a0c7e (#34446) only the compiled bundle was modified. This meant that anyone who ran yarn build in the actioncable directory would then see a dirty git status indicating changes to the compiled bundle, despite not having made any changes to the actioncable javascript source code. We fixed that particular inconsistency in a4c2758 (#34475). However, the same problem could reoccur.

To address this, I've added a new test to enforce that actioncable's compiled javascript bundle is in sync with the source code. When the compiled bundle is in sync with the source code, the test will pass:

$ bundle exec ruby -Itest test/javascript_package_test.rb
Run options: --seed 19308

# Running:

yarn run v1.12.3
$ yarn lint && bundle exec rake assets:codegen
$ eslint app/javascript
$ rollup --config rollup.config.js

app/javascript/action_cable/index.js → app/assets/javascripts/action_cable.js...
created app/assets/javascripts/action_cable.js in 762ms
✨  Done in 6.35s.
.

Finished in 7.130345s, 0.1402 runs/s, 0.1402 assertions/s.
1 runs, 1 assertions, 0 failures, 0 errors, 0 skips

However, if the two are not in sync, the test will fail. For example, if you were to apply the following patch (which only updates the source code):

diff --git a/actioncable/app/javascript/action_cable/adapters.js b/actioncable/app/javascript/action_cable/adapters.js
index 4de8131438..d38d9a6a0b 100644
--- a/actioncable/app/javascript/action_cable/adapters.js
+++ b/actioncable/app/javascript/action_cable/adapters.js
@@ -1,4 +1,5 @@
 export default {
+  foo: self.foo,
   logger: self.console,
   WebSocket: self.WebSocket
 }

the test would then fail like this:

$ bundle exec ruby -Itest test/javascript_package_test.rb
Run options: --seed 26377

# Running:

yarn run v1.12.3
$ yarn lint && bundle exec rake assets:codegen
$ eslint app/javascript
$ rollup --config rollup.config.js

app/javascript/action_cable/index.js → app/assets/javascripts/action_cable.js...
created app/assets/javascripts/action_cable.js in 776ms
✨  Done in 5.55s.
F

Failure:
JavascriptPackageTest#test_compiled_code_is_in_sync_with_source_code [test/javascript_package_test.rb:16]:
--- expected
+++ actual
@@ -3,6 +3,7 @@
 })(this, function(exports) {
   \"use strict\";
   var adapters = {
+    foo: self.foo,
     logger: self.console,
     WebSocket: self.WebSocket
   };

rails test test/javascript_package_test.rb:9

Finished in 5.837403s, 0.1713 runs/s, 0.1713 assertions/s.
1 runs, 1 assertions, 1 failures, 0 errors, 0 skips

Thus, the actioncable test suite will now prevent "the compiled bundle is out of sync" issues going forward.

@rmacklin rmacklin force-pushed the enforce-that-actioncable-compiled-bundle-is-in-sync-with-source-code branch from 64b237b to a4aaaaf Compare October 16, 2019 20:48
@kaspth kaspth requested a review from javan October 16, 2019 21:07
We have run into issues in the past where the actioncable compiled
javascript bundle got out of sync with the source code. For example, in
30a0c7e only the compiled bundle was
modified. This meant that anyone who ran `yarn build` in the actioncable
directory would then see a dirty git status indicating changes to the
compiled bundle, despite not having made any changes to the actioncable
javascript source code. We fixed that particular inconsistency in
a4c2758. However, the same problem
could reoccur.

To address this, I've added a new test to enforce that actioncable's
compiled javascript bundle is in sync with the source code. When the
compiled bundle is in sync with the source code, the test will pass:

    $ bundle exec ruby -Itest test/javascript_package_test.rb
    Run options: --seed 19308

    # Running:

    yarn run v1.12.3
    $ yarn lint && bundle exec rake assets:codegen
    $ eslint app/javascript
    $ rollup --config rollup.config.js

    app/javascript/action_cable/index.js → app/assets/javascripts/action_cable.js...
    created app/assets/javascripts/action_cable.js in 762ms
    ✨  Done in 6.35s.
    .

    Finished in 7.130345s, 0.1402 runs/s, 0.1402 assertions/s.
    1 runs, 1 assertions, 0 failures, 0 errors, 0 skips

However, if the two are not in sync, the test will fail. For example, if
you were to apply the following patch (which only updates the source
code):

```
diff --git a/actioncable/app/javascript/action_cable/adapters.js b/actioncable/app/javascript/action_cable/adapters.js
index 4de8131438..d38d9a6a0b 100644
--- a/actioncable/app/javascript/action_cable/adapters.js
+++ b/actioncable/app/javascript/action_cable/adapters.js
@@ -1,4 +1,5 @@
 export default {
+  foo: self.foo,
   logger: self.console,
   WebSocket: self.WebSocket
 }
```

the test would then fail like this:

    $ bundle exec ruby -Itest test/javascript_package_test.rb
    Run options: --seed 26377

    # Running:

    yarn run v1.12.3
    $ yarn lint && bundle exec rake assets:codegen
    $ eslint app/javascript
    $ rollup --config rollup.config.js

    app/javascript/action_cable/index.js → app/assets/javascripts/action_cable.js...
    created app/assets/javascripts/action_cable.js in 776ms
    ✨  Done in 5.55s.
    F

    Failure:
    JavascriptPackageTest#test_compiled_code_is_in_sync_with_source_code [test/javascript_package_test.rb:16]:
    --- expected
    +++ actual
    @@ -3,6 +3,7 @@
     })(this, function(exports) {
       \"use strict\";
       var adapters = {
    +    foo: self.foo,
         logger: self.console,
         WebSocket: self.WebSocket
       };

    rails test test/javascript_package_test.rb:9

    Finished in 5.837403s, 0.1713 runs/s, 0.1713 assertions/s.
    1 runs, 1 assertions, 1 failures, 0 errors, 0 skips

Thus, the actioncable test suite will now prevent "the compiled bundle
is out of sync" issues going forward.
@rmacklin rmacklin force-pushed the enforce-that-actioncable-compiled-bundle-is-in-sync-with-source-code branch from a4aaaaf to 06a9473 Compare October 18, 2019 03:46
@kaspth
Copy link
Contributor

kaspth commented Oct 19, 2019

Merged in 2c7ec81, thanks so much!

@kaspth kaspth closed this Oct 19, 2019
@rmacklin rmacklin deleted the enforce-that-actioncable-compiled-bundle-is-in-sync-with-source-code branch October 19, 2019 00:33
@rmacklin
Copy link
Contributor Author

rmacklin commented Oct 19, 2019

Thanks for reviewing! I was going to add a similar test in activestorage since the same issue appeared there (per #34473 (comment)) but I was waiting to hear feedback on the approach first. Since this is already merged I take it we're comfortable with this approach, so I'll open a new PR to handle activestorage.

Edit:
Here's the corresponding activestorage PR: #37508

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants