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

Build action_cable.js with Blade #25119

Merged
merged 2 commits into from May 24, 2016

Conversation

Projects
None yet
5 participants
@javan
Member

javan commented May 23, 2016

  • Compiles action_cable.js with Blade, which essentially uses the same Sprockets setup we had in the rake task. No need to duplicate.
  • Uses sprocket-export's #= export directive to replace the somewhat incomplete implementation

/cc @maclover7 @jeremy

@javan javan added the actioncable label May 23, 2016

@@ -1,3 +1,4 @@
#= export ActionCable

This comment has been minimized.

@matthewd

matthewd May 23, 2016

Member

Does this make blade a runtime requirement for anyone running out of a git clone?

This comment has been minimized.

@javan

javan May 23, 2016

Member

No. Unknown Sprockets directives are ignored so without Blade the default behavior applies (setting window.ActionCable).

@matthewd

View changes

actioncable/blade.yml Outdated
logical_paths:
- action_cable.js
path: lib/assets/compiled
js_compressor: uglifier

This comment has been minimized.

@matthewd

matthewd May 23, 2016

Member

I think I'd prefer to keep this in a form amenable to hackery -- I know it's Bad, but I'll often open a file in an installed gem to install a temporary puts; retaining the ability to do the same with console.log seems desirable... especially as it doesn't really matter -- it'll be compressed eventually either way.

(Bonus argument: if we pre-compress, sprockets + source-maps will leave this file unpleasant to debug even when everything else is nice and shiny.)

This comment has been minimized.

@javan

javan May 23, 2016

Member

For that kind of hackery, you could point to the uncompiled action_cable.coffee for inserting your console.logs, etc.

especially as it doesn't really matter -- it'll be compressed eventually either way

That's true if your app compresses assets, but it's not a given. Especially considering people can install via npm and use action_cable.js outside of Rails / Sprockets.

I don't feel strongly that it should be compressed given its relatively small file size, but it feels like the right thing to do for a distributed .js file. FWIW, Turbolinks is compressed using the same setup.

This comment has been minimized.

@matthewd

matthewd May 23, 2016

Member

For that kind of hackery, you could point to the uncompiled action_cable.coffee

The uncompiled file isn't there, though.

This comment has been minimized.

@vipulnsward

vipulnsward May 23, 2016

Member

For that kind of hackery, you could point to the uncompiled action_cable.coffee for inserting your console.logs, etc.

On npm most would prefer the JS/ES6 version. Even if they would use coffee version we don't ship with it right now on npm.

That's true if your app compresses assets, but it's not a given. Especially considering people can install via npm and use action_cable.js outside of Rails / Sprockets.

Anyone doing doing this will already be using compression by some other means.

Here's how our current npm package looks-

Here's some other examples from popular libraries-

Most of them contain uncompressed + compressed version. Its ok to just ship with compressed one, since its essential for things listed by Matthew:

  • sourcemaps
  • edit/check source code of actual library
  • source linking of methods, etc in editors

I don't feel strongly that it should be compressed given its relatively small file size, but it feels like the right thing to do for a distributed .js file

As seen above many libraries ship with both, so that they can be used as needed. For us its ok to ship just with unminified version as well.

This comment has been minimized.

@javan

javan May 23, 2016

Member

Ah, right. Forgot we only include lib in the gem.

Either way, I'm not sure your use case of editing an installed gem justifies distributing a larger than necessary .js file.

This comment has been minimized.

@javan

javan May 23, 2016

Member

I removed js compression for now (javan@27ae7c3), but I think we have it backwards. The reasons listed for using an uncompressed version are all development concerns so, if anything, we should provide an additional uncompressed version for debugging.

Turbolinks is part of the default stack and compresses its source so we're currently inconsistent about the js we bundle.

it doesn't really matter -- it'll be compressed eventually either way
Anyone doing doing this will already be using compression by some other means

You both seem pretty confident about this for some reason. Have you seen the... internet? 😁

This comment has been minimized.

@vipulnsward

vipulnsward May 23, 2016

Member

@javan ideally we should include both, as done by others, and provide option to use either one.
For me, it would be really important to have uncompressed version to check out the source, editor linking, source maps, etc. Again, this is because of how we develop right now, CS->JS. Other libs just include their source too, and
provide dist versions of compressed/uncompressed versions, minus the noise like tests.

Right now we are on edge and ironing things out, I sometimes need to do manual exports for npm(not ideal), for making it work in some occasions, or some others might need to, to suit their export systems, commonjs, amd, etc.

@maclover7

View changes

actioncable/Rakefile Outdated
dir = File.dirname(__FILE__)
task :default => :test
task :package => "assets:compile"
task "package:clean" => "assets:clean"
task "package:clean"

This comment has been minimized.

@maclover7

maclover7 May 23, 2016

Member

Don't we still we need to define this task, and provide a way for the compiled file to be deleted? 😬

This comment has been minimized.

@rafaelfranca

This comment has been minimized.

@javan

javan May 23, 2016

Member

All the other gems define an empty "package:clean" task. Introduced in d6f2000. I think Action Cable was the only one to implement it. Can probably remove them all now. @matthewd?

This comment has been minimized.

@rafaelfranca

rafaelfranca May 23, 2016

Member

Don't we still need to delete the compiled files?

This comment has been minimized.

@matthewd

matthewd May 23, 2016

Member

It was indeed the only one, because it's the only component that currently produces a temporary file to clean up. I don't see how a change in the mechanism we use to generate the file obviates such a task... but I agree there's no point leaving it here empty.

This comment has been minimized.

@javan

This comment has been minimized.

@javan

javan May 23, 2016

Member

Removed package:clean in af777bf6ae67c8ec3ef200415d238614fefef68e and updated assets:compile to ensure a clean path in db84cb439a2c85cdace279451caad4bf40f7eb33.

@javan javan force-pushed the javan:actioncable/blade-build branch 2 times, most recently May 23, 2016

@javan

This comment has been minimized.

Member

javan commented May 24, 2016

I updated Blade to output the build process and handle cleaning the dist path:

$ bundle exec rake assets:compile
Building assets…
[removed] lib/assets/compiled/action_cable.js
[created] lib/assets/compiled/action_cable.js
@vipulnsward

View changes

Gemfile.lock Outdated
eventmachine (1.2.0.1)
eventmachine (1.2.0.1-x64-mingw32)
eventmachine (1.2.0.1-x86-mingw32)
execjs (2.6.0)

This comment has been minimized.

@vipulnsward

vipulnsward May 24, 2016

Member

@javan I see some downgrades, are they intentional from blade? or the dropping of coffee-script versions, etc.

This comment has been minimized.

@javan

javan May 24, 2016

Member

These dependencies were all added recently in 61483b1 when Blade 0.5.1 was introduced. It turns out Thin (a dependency of Blade) doesn't work with event machine 1.2.0 so I backed away from it in javan/blade@62c8cb0. Blade also removed exact dependencies on coffee-script and sprockets to reduce conflicts with apps that have tighter requirements. So if we want specific versions, we should declare them.

@javan javan force-pushed the javan:actioncable/blade-build branch to d12209c May 24, 2016

Remove package:clean task
Introduced in d6f2000 and was only used by Action Cable. Now handled by Action Cable’s assets:compile task.

@rafaelfranca rafaelfranca merged commit 04a0e89 into rails:master May 24, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@rafaelfranca

This comment has been minimized.

Member

rafaelfranca commented Jul 4, 2016

Backported in 45083d8

rafaelfranca added a commit that referenced this pull request Jul 4, 2016

Merge pull request #25119 from javan/actioncable/blade-build
Build action_cable.js with Blade
@javan

This comment has been minimized.

Member

javan commented Jul 4, 2016

Thanks, @rafaelfranca! Is it possible to release actioncable-5.0.1 independently (to fix #25649) or are the Rails gems always released together?

@rafaelfranca

This comment has been minimized.

Member

rafaelfranca commented Jul 4, 2016

They are always released together. If you see the rails.gemspec we ask for the same version of the subgems. https://github.com/rails/rails/blob/master/rails.gemspec#L28.

If it is a critical fix we can do a full release early, but this is more something to @matthewd decide since he is the release manager of the next release.

@javan

This comment has been minimized.

Member

javan commented Jul 5, 2016

Cool. I don't think it's critical.

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