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

Add Chrome Extension #151

Merged
merged 8 commits into from Aug 4, 2015

Conversation

@sh19910711
Copy link

commented Jul 20, 2015

This pull-request contains Chrome Extension and some preparations for the browser extensions.

1. Use a static method to send XHRs
  • Use a static method to send XHRs (ref: 937d220)
    • Add "REPLConsole.request()"
      • We can use this static method from instance methods and external scripts
      • And we can override this method by reassigning if needed
    • Add "REPLConsole.XMLHttpRequest" for browser extensions
  • Add tests for REPLConsole#commandHandle() (ref: 20ae7e5)
    • This is also testing REPLConsole.request()
2. Other preparations for the browser extensions
  • Add support for CommonJS (ref: c820b09)
    • For the Firefox Extension.
  • Add full-screen mode for browser extensions (ref: 4cf06fb)
    • CSS-based
    • For the browser extensions
3. Add Chrome Extension
  • Add source code of Chrome Extension (ref: 84329e0)
    • Show the console in the DevTools
  • Create a build environment for Chrome Extension (ref: 63279d9)
    • Use Gulp to build the browser extensions

About the extension, we can try the development version by the following commands:

$ git clone https://github.com/rails/web-console.git
$ cd web-console/brx
$ bundle install
$ bundle exec rake ext:chrome:run

Now I'm looking for an edge case for the extension, so welcome any feedback from anyone at any time.

And hello @gsamokovarov, could you review this pull-request, please?

Thank you.

@sh19910711 sh19910711 force-pushed the sh19910711:patch/0017/chrome-extension branch from 63279d9 to 5366360 Jul 23, 2015

@@ -0,0 +1,66 @@
# Tasks to build chrome extension.

This comment has been minimized.

Copy link
@gsamokovarov

gsamokovarov Jul 23, 2015

Collaborator

Can we build the extensions with Ruby tooling? E.g. rake.

@@ -0,0 +1,40 @@
require "pathname"

This comment has been minimized.

Copy link
@gsamokovarov

gsamokovarov Jul 23, 2015

Collaborator

Don't we need a similar file for the testing? Maybe we can put it in lib/web_console and use as an library instead.

@@ -0,0 +1,12 @@
# Web Console Browser Extensions

This comment has been minimized.

Copy link
@gsamokovarov

gsamokovarov Jul 23, 2015

Collaborator

We can we rename the brx folder to chrome so its easier to guess what it contains.

@@ -446,6 +450,24 @@ REPLConsole.installInto = function(id, options) {
// It allows to operate the current session from the other scripts.
REPLConsole.currentSession = null;

REPLConsole.XMLHttpRequest = typeof XMLHttpRequest === 'undefined' ? null : XMLHttpRequest;

This comment has been minimized.

Copy link
@gsamokovarov

gsamokovarov Jul 23, 2015

Collaborator

When can we miss XMLHttpRequest these days? Is it in the chrome sandbox?

This comment has been minimized.

Copy link
@sh19910711

sh19910711 Jul 24, 2015

Author

This line is for the Firefox Add-on. It doesn't have XMLHttpRequest as default, and so it needs to require a compatible module (see also here: http://git.io/vYW6X).

This comment has been minimized.

Copy link
@gsamokovarov

gsamokovarov Jul 24, 2015

Collaborator

Oh, cool. Can you put a comment on top of it?

This comment has been minimized.

Copy link
@sh19910711

sh19910711 Jul 24, 2015

Author

Yeah, I will do it!

)
};

new Background;

This comment has been minimized.

Copy link
@gsamokovarov

gsamokovarov Jul 23, 2015

Collaborator

If we don't get to keep the instance around, why instantiate it in the first place? If its just for the side effect, should that be an object or a bag of functions?

@sh19910711 sh19910711 force-pushed the sh19910711:patch/0017/chrome-extension branch 3 times, most recently from 0a62f07 to f60f126 Jul 26, 2015

@sh19910711

This comment has been minimized.

Copy link
Author

commented Jul 26, 2015

Updated:

  • Rename directories
    • brx/ => extensions/
    • crx/ => chrome/
  • Add FakeMiddleware class for testing and the browser extensions (ref: af0301b)
  • Add a feature to hide an existing console element on the page when the console is displayed in the panel of Chrome DevTools
    • For that, add REPLConsole#uninstall() (ref: 2f44b6a)

The Rakefile is not finished yet

GIF - Hide existing console

GIF

Thanks.

@gsamokovarov

This comment has been minimized.

Copy link
Collaborator

commented Jul 26, 2015

Great! Thanks for the update.

@simkessy

This comment has been minimized.

Copy link

commented Jul 27, 2015

This looks really awesome. I'm running rails through Vagrant through, so Chrome is on a localhost and rails is running in a VM. How would I set this up on my current system?

@sh19910711

This comment has been minimized.

Copy link
Author

commented Jul 27, 2015

@simkessy Thank you. The extension can detect Web Console by a response header, so I think there is no need to set up if you see the default console. All you have to do is to install the extension (and you might need to do gem "web-console", "~>2.2.0" or something perhaps).

@sh19910711 sh19910711 force-pushed the sh19910711:patch/0017/chrome-extension branch from f60f126 to 563319e Jul 27, 2015

@sh19910711

This comment has been minimized.

Copy link
Author

commented Jul 27, 2015

Updated:

  • Use Rake instead of Gulp to build the browser extensions
  • Add rake tasks for the Chrome Extension
    • ext:chrome is to build the extension
    • ext:chrome:run: is to launch a browser with the extension

@gsamokovarov Hello, could you review the rake tasks again? (ref: 563319e)

Thank you!

require "web_console"

module WebConsole
class FakeMiddleware

This comment has been minimized.

Copy link
@gsamokovarov

gsamokovarov Jul 27, 2015

Collaborator

Can this live in lib/web_console/testing/fake_middleware.rb? This is to follow the example of other Rails component grouping their testing utilities in similar module.

@@ -0,0 +1,19 @@
require "web_console/fake_middleware"

This comment has been minimized.

Copy link
@gsamokovarov

gsamokovarov Jul 27, 2015

Collaborator

Instead of executing this through an external process, let's make this a library and use it as Ruby code in the Rakefile. I'm kinda troubled about where this should live. We can put it in lib/web_console/testing/erb_helper for now.

This comment has been minimized.

Copy link
@sh19910711

sh19910711 Jul 28, 2015

Author

@gsamokovarov OK, I will try them! Thanks.

@sh19910711 sh19910711 force-pushed the sh19910711:patch/0017/chrome-extension branch from 563319e to 59fe2f3 Jul 28, 2015

@sh19910711

This comment has been minimized.

Copy link
Author

commented Jul 28, 2015

Updated:

  • Move erb_helper.rb and fake_middleware.rb into lib/web_console/testing/
  • Add ERBHelper class to pre-compile templates (ref: 59fe2f3)
    • We can build the extension faster!

@gsamokovarov Any other points you have?

Thanks.

"crx": "^3.0.2"
},
"scripts": {
"crx": "npm install && node \"$(npm bin)/gulp\" crx/run"

This comment has been minimized.

Copy link
@gsamokovarov

gsamokovarov Jul 28, 2015

Collaborator

We don't have gulp anymore, right?

var self = this;
var params = 'input=' + encodeURIComponent(line);
cb = cb || function() {};

This comment has been minimized.

Copy link
@gsamokovarov

gsamokovarov Jul 28, 2015

Collaborator

s/cb/callback :)

@@ -0,0 +1,25 @@
#!/bin/sh

This comment has been minimized.

Copy link
@gsamokovarov

gsamokovarov Jul 28, 2015

Collaborator

Can we have this dir as script (singular)? This is to match the default rails project structure, where this folder is named in singular.

@sh19910711 sh19910711 force-pushed the sh19910711:patch/0017/chrome-extension branch from 59fe2f3 to ae8bbde Jul 28, 2015

@sh19910711

This comment has been minimized.

Copy link
Author

commented Jul 28, 2015

Updated again:

  • Rename scripts => script
  • Move FakeMiddleware into the top-level (it means WebConsole::FakeMiddleware => FakeMiddleware)
  • Squash commits of the rake tasks

Thanks again.

require 'web_console/testing/fake_middleware'

# This class is to pre-compile 'templates/*.erb'.
class ERBHelper

This comment has been minimized.

Copy link
@gsamokovarov

gsamokovarov Jul 28, 2015

Collaborator

ERBPrecompiler

This comment has been minimized.

Copy link
@gsamokovarov

gsamokovarov Jul 28, 2015

Collaborator

Oh, also, please put it inside of the WebConsole::Testing namespace. Its more predictable to have the web_console/testing/erb_helper class/module in this namespace. You can call it with the fully qualified name wherever its referenced now. Same applies to the FakeMiddleware.

This comment has been minimized.

Copy link
@sh19910711

sh19910711 Jul 28, 2015

Author

OK, I will do it :-)

def root(path = '')
Pathname(File.expand_path '../../../../', __FILE__).join path
end

This comment has been minimized.

Copy link
@gsamokovarov

gsamokovarov Jul 28, 2015

Collaborator

Why you wanna do that? Can you put a comment on it?

require "web_console"

class FakeMiddleware
DEFAULT_HEADERS = {"Content-Type" => "application/javascript"}

This comment has been minimized.

Copy link
@gsamokovarov

gsamokovarov Jul 28, 2015

Collaborator

Spaces around { and [. Most of the rails codebase follows that.

end

def build
@erb.result binding

This comment has been minimized.

Copy link
@gsamokovarov

gsamokovarov Jul 28, 2015

Collaborator

I'd also prefer to have parentheses when calling non DSL-ish methods. Again, to follow the rest of Rails' codebase.

} else {
self.writeError(response.output);
}
cb();

This comment has been minimized.

Copy link
@gsamokovarov

gsamokovarov Jul 28, 2015

Collaborator

Should the callback know whether the request succeeded or failed?

Rakefile Outdated

namespace :lib do
templates = Pathname('lib/web_console/templates')
tmplib = rootdir.join('tmp/lib/')

This comment has been minimized.

Copy link
@gsamokovarov

gsamokovarov Jul 28, 2015

Collaborator

Alignment.

@sh19910711 sh19910711 force-pushed the sh19910711:patch/0017/chrome-extension branch from ae8bbde to 0f64708 Jul 29, 2015

@sh19910711

This comment has been minimized.

Copy link
Author

commented Jul 29, 2015

Updated:

  • Do callback(result, response) in the REPLConsole#commandHandle() (ref: 5d66fa3)
  • Fix the new hash syntax in the Rakefile ✂️
  • Use WebConsole::Testing
  • And rename ERBHelper to ERBPrecompiler

Thanks!

@sh19910711

This comment has been minimized.

Copy link
Author

commented Jul 29, 2015

Forgot to write, I added WebConsole.gem_root into the lib/web_console.rb to get path from root directory (ref: 0f64708#diff-4ffc3efe29c6bb1b76c331c9cffe6b67R22).

require "action_dispatch"
require "web_console"

module WebConsole::Testing

This comment has been minimized.

Copy link
@gsamokovarov

gsamokovarov Jul 29, 2015

Collaborator

I would prefer it written like:

module WebConsole
  module Testing
    class FakeMiddleware
    end
  end
end

That way, the file that is required first will create the WebConsole::Testing module and the other files will just reopen it. You also get better Module.nesting and can reference constants in WebConsole relatively as well. Same for the precompiler.

def initialize(path)
@erb = ERB.new(File.read(path))
@view = FakeMiddleware.new(
view_path: WebConsole.gem_root('lib/web_console/templates'),

This comment has been minimized.

Copy link
@gsamokovarov

gsamokovarov Jul 29, 2015

Collaborator

I'd prefer gem_root to just return a Pathname object. If you need to join on it, you can call #join.

@sh19910711 sh19910711 force-pushed the sh19910711:patch/0017/chrome-extension branch from 0f64708 to 221da56 Jul 29, 2015

@sh19910711

This comment has been minimized.

Copy link
Author

commented Jul 29, 2015

Updated again.

I'd prefer gem_root to just return a Pathname object. If you need to join on it, you can call #join.

Yeah, that makes sense.

You also get better Module.nesting and can reference constants in WebConsole relatively as well. Same for the precompiler.

Now I get it.

Thanks!

Rakefile Outdated

task build: [ extdir, 'lib:templates' ] do
cd rootdir do
cp_r [ 'chrome/html', 'chrome/js', 'chrome/manifest.json', 'img/', 'tmp/lib/' ], dist

This comment has been minimized.

Copy link
@sh19910711

sh19910711 Jul 30, 2015

Author

This line ends up copying files which should be ignored (e.g., vim swap files). I will rewrite this task with git ls-files or something else.

This comment has been minimized.

Copy link
@gsamokovarov

gsamokovarov Jul 30, 2015

Collaborator

👍

@sh19910711 sh19910711 force-pushed the sh19910711:patch/0017/chrome-extension branch 2 times, most recently from f709292 to 49acec6 Jul 31, 2015

@sh19910711

This comment has been minimized.

Copy link
Author

commented Jul 31, 2015

Updated (diff for the Rakefile: 221da56...2d350dc):

Thanks!

@sh19910711

This comment has been minimized.

Copy link
Author

commented Jul 31, 2015

Don't require "web-console" on fake_middleware.rb

Oh, this also causes NoMethodError for the WebConsole.gem_root. I will fix it.

@sh19910711 sh19910711 force-pushed the sh19910711:patch/0017/chrome-extension branch from 49acec6 to f79075b Jul 31, 2015

@sh19910711

This comment has been minimized.

Copy link
Author

commented Jul 31, 2015

Updated again:

  • Move WebConsole.gem_root to WebConsole::Testing::Helper.gem_root (diff is here: 33e6a59)
    • It means to limit the access to the method for testing and similar things

And thanks again.

Rakefile Outdated
cp_r [ 'img/', 'tmp/lib/' ], dist
`cd chrome && git ls-files`.split("\n").each do |src|
dest = dist.join(src)
mkdir_p dest.dirname unless Dir.exists?(dest.dirname)

This comment has been minimized.

Copy link
@gsamokovarov

gsamokovarov Jul 31, 2015

Collaborator

I think mkdir_p will check if the dir exist and won't create it at all, so you can save yourself the condition.

Create a build environment for the Chrome Extension
For the development:

```
$ git clone https://github.com/rails/web-console.git
$ cd web-console
$ bundle install
$ bundle exec rake ext:chrome:run
=> Chrome will be launched with the extension.
```

And provide some rake tasks for the extension:

- "ext:chrome" is to build the extension
- "ext:chrome:run" is to launch a browser with the extension
- "ext:chrome:zip" is to generate .zip
- "ext:chrome:crx" is to generate .crx

@sh19910711 sh19910711 force-pushed the sh19910711:patch/0017/chrome-extension branch from f79075b to 7098a7e Jul 31, 2015

@sh19910711

This comment has been minimized.

Copy link
Author

commented Jul 31, 2015

Updated again*2 (diff: f79075b...ece32ec):

  • Remove the unless condition
  • Fix indent of private methods of the FakeMiddleware class (>>)
  • Remove attr_readers of the FakeMiddleware class
@gsamokovarov

This comment has been minimized.

Copy link
Collaborator

commented Aug 4, 2015

@sh19910711 Do you have anything to add here? There are areas which we can improve, but I think it's a decent start.

@sh19910711

This comment has been minimized.

Copy link
Author

commented Aug 4, 2015

@gsamokovarov No, I don't. I think so too :-)

gsamokovarov added a commit that referenced this pull request Aug 4, 2015

@gsamokovarov gsamokovarov merged commit 4e46969 into rails:master Aug 4, 2015

1 check passed

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

This comment has been minimized.

Copy link
Author

commented Aug 4, 2015

@gsamokovarov Thanks for merging 🙇

@gsamokovarov gsamokovarov added the gsoc label Aug 7, 2015

@gsamokovarov

This comment has been minimized.

Copy link
Collaborator

commented Aug 7, 2015

Ref. #121

@gsamokovarov

This comment has been minimized.

Copy link
Collaborator

commented Aug 11, 2015

Hey @sh19910711, we seem to have forgotten to include the renamed script/ folder in version control. Can you issue a PR with it?

@sh19910711

This comment has been minimized.

Copy link
Author

commented Aug 12, 2015

Hey. That folder might be "extensions/script"?

@simkessy

This comment has been minimized.

Copy link

commented Sep 3, 2015

@simkessy Thank you. The extension can detect Web Console by a response header, so I think there is no need to set up if you see the default console. All you have to do is to install the extension (and you might need to do gem "web-console", "~>2.2.0" or something perhaps).

@sh19910711 I'm trying to follow the install instructions but I'm not sure if I'm suppose to be cloning the extension locally or in vagrant? So far I'm tried to do everything in vagrant and I get this error when trying to bundle

Bundler could not find compatible versions for gem "rack":
  In Gemfile:
    rails (>= 0) ruby depends on
      actionpack (= 5.0.0.alpha) ruby depends on
        rack (~> 2.x) ruby
Could not find gem 'rack (~> 2.x) ruby', which is required by gem 'actionpack (= 5.0.0.alpha) ruby', in

any of the sources.
@sh19910711

This comment has been minimized.

Copy link
Author

commented Sep 4, 2015

@simkessy Hi. It's better to clone the extension locally if you use it in Chrome on the real machine.

So far I'm tried to do everything in vagrant and I get this error when trying to bundle

I got the same error on the master branch. I think we can pass it by specifying the Gemfile of Web Console (like that bundle install --gemfile=./gemfiles/Gemfile-4-2-stable or something else) for now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.