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

Restore action_cable.js UMD module support. Fixes #28366 #28385

Merged
merged 1 commit into from Mar 13, 2017

Conversation

Projects
None yet
4 participants
@javan
Member

javan commented Mar 11, 2017

Fix for #28366

774be3e bumped Blade from 0.6.1 to 0.7.0, which dropped sprockets-export from Gemfile.lock (Blade 0.7.0 no longer includes sprockets-export as a dependency). Doing so silently dropped action_cable.js's UMD module support, essentially breaking it as an npm package.

This change restores sprockets-export and adds a new rake task that validates action_cable.js to help ensure this doesn't happen again.

/cc @matthewd

@matthewd

This comment has been minimized.

Member

matthewd commented Mar 11, 2017

❤️

We already depend on npm being available in the build/release pipeline.. would it be worth extending this validation task to actually load it via nodejs somehow, and check.. something?

@javan

This comment has been minimized.

Member

javan commented Mar 11, 2017

Yeah, if node is available we could do something like:

--- a/actioncable/Rakefile
+++ b/actioncable/Rakefile
@@ -50,6 +50,11 @@ namespace :assets do
 
     raise "[FAIL] #{file} is missing" unless pathname.exist?
     raise "[FAIL] #{file} isn't a UMD module" unless pathname.read =~ umd_pattern
+
+    require "open3"
+    stdout, stderr, status = Open3.capture3("node", "--print", "window = {}; require('#{dir}');")
+    raise "[FAIL] #{file} can't be required as a module:\n#{stderr}" unless status.success?
+

Which would fail like:

$ bundle exec rake package
Building assets…
[removed] lib/assets/compiled/action_cable.js
[created] lib/assets/compiled/action_cable.js
rake aborted!
[FAIL] lib/assets/compiled/action_cable.js can't be required as a module:
/Users/javan/Projects/rails/actioncable/lib/assets/compiled/action_cable.js:61
  ActionCable.ConnectionMonitor = (function() {
  ^

ReferenceError: ActionCable is not defined
    at Object.<anonymous> (/Users/javan/Projects/rails/actioncable/lib/assets/compiled/action_cable.js:61:3)
    at Object.<anonymous> (/Users/javan/Projects/rails/actioncable/lib/assets/compiled/action_cable.js:192:4)
    at Module._compile (module.js:571:32)
    at Object.Module._extensions..js (module.js:580:10)
    at Module.load (module.js:488:32)
    at tryModuleLoad (module.js:447:12)
    at Function.Module._load (module.js:439:3)
    at Module.require (module.js:498:17)
    at require (internal/module.js:20:19)
    at [eval]:1:14
/Users/javan/Projects/rails/actioncable/Rakefile:55:in `block (2 levels) in <top (required)>'

Happy to commit that if you'd like.

@matthewd

This comment has been minimized.

Member

matthewd commented Mar 11, 2017

Sounds good 👍

@javan javan force-pushed the javan:actioncable/fix-npm-package branch to 2d2b302 Mar 11, 2017

@javan

This comment has been minimized.

Member

javan commented Mar 11, 2017

Updated and squashed. Output now looks like:

$ bundle exec rake package
Building assets…
[removed] lib/assets/compiled/action_cable.js
[created] lib/assets/compiled/action_cable.js
[verify] lib/assets/compiled/action_cable.js exists [OK]
[verify] lib/assets/compiled/action_cable.js is a UMD module [OK]
[verify] /Users/javan/Projects/rails/actioncable can be required as a module [OK]

@guilleiguaran guilleiguaran merged commit a5746cb into rails:master Mar 13, 2017

2 checks passed

codeclimate no new or fixed issues
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment