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

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

Merged
merged 1 commit into from
Mar 13, 2017

Conversation

javan
Copy link
Contributor

@javan 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
Copy link
Member

❤️

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
Copy link
Contributor Author

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
Copy link
Member

Sounds good 👍

@javan javan force-pushed the actioncable/fix-npm-package branch from 824f27b to 2d2b302 Compare March 11, 2017 21:11
@javan
Copy link
Contributor Author

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

4 participants