Click handler refactoring proposal #274

stefanbuck · 2017-02-01T23:29:36Z

Once @josephfrazier suggested to combine the plugin click handler (aka resolver) with the actual plugin. At this time, I was assuming that we can benefit from reusing those resolver from one plugin in others. Now, after a couple of months of adding more and more languages, it shows up that only a small portion ( git-url, relative-file and live-resolver-query) of resolver are reused.

Exemplary I refactored the Docker and Python plugin to show you how it could work. I'll polish the PR if you agree with the solution. I'm looking forward to your feedback.

Please, review the PR with ?w=1 to ignore whitespace changes.

TODO

josephfrazier · 2017-02-02T22:29:00Z

This is a big one, so I'll try to take a look at it over the weekend. Thanks for the heads-up about the whitespace changes.

stefanbuck · 2017-02-02T23:17:39Z

No rush 😉

josephfrazier

Ok, I got a chance to review this. I really like the changes overall! It's exciting to see my proposal in #86 taking shape.

I added a handful of small comments about variable names and function signatures, but this looks good otherwise 👍

PS: It turns out that using ?w=1 to ignore whitespace changes makes it impossible to add inline comments (isaacs/github#394). That didn't cause much trouble though, as I had already read the code without the whitespace changes, before I tried to start adding comments.

josephfrazier · 2017-02-06T18:59:28Z

lib/click-handler.js

-        };
-      });
-    }
+  const pluginName = dataAttr._pluginName;


I think we should rename _pluginName to simply pluginName, since there's no previously existing pluginName variable that it would be shadowing.

josephfrazier · 2017-02-06T19:01:12Z

lib/insert-link.js

-  const dataAttr = {};
+function buildDataAttr(_pluginName, data, match) {
+  const dataAttr = {
+    _pluginName,


EDIT: See this comment first: https://github.com/OctoLinker/browser-extension/pull/274/files#r99663118

If you agree with my comment at https://github.com/OctoLinker/browser-extension/pull/274/files#r99657441, we should also change _pluginName to pluginName on this line (and in the function definition).

josephfrazier · 2017-02-06T19:03:15Z

lib/click-handler.js

-      });
-    }
+  const pluginName = dataAttr._pluginName;
+  const resolver = pluginManager.getClickHandler(pluginName);


I think we should rename getClickHandler to getResolver, since that more accurately describes what the return value does (and matches the variable name used here).

josephfrazier · 2017-02-06T19:04:21Z

lib/plugin-manager.js

    this._plugins = buildPluginCache(plugins);
  }

+  getClickHandler(pluginName) {


If you agree with my comment at https://github.com/OctoLinker/browser-extension/pull/274/files#r99658228, we should also change getClickHandler to getResolver here.

josephfrazier · 2017-02-06T19:05:31Z

lib/plugin-manager.js

+      return;
+    }
+
+    return plugin.clickHandler;


In the spirit of https://github.com/OctoLinker/browser-extension/pull/274/files#r99658228, I think clickHandler should instead be resolve on this line and in the previous conditional.

josephfrazier · 2017-02-06T19:06:51Z

lib/plugins/docker/index.js

@@ -4,13 +4,25 @@ import preset from '../../pattern-preset';

 export default class Docker {

+  static clickHandler({ image }) {


If you agree with my comment at https://github.com/OctoLinker/browser-extension/pull/274/files#r99658736, we should also change clickHandler to resolve here.

josephfrazier · 2017-02-06T19:07:05Z

lib/plugins/python/index.js


 export default class Python {

+  static clickHandler({ path, target }) {


If you agree with my comment at https://github.com/OctoLinker/browser-extension/pull/274/files#r99658736, we should also change clickHandler to resolve here.

josephfrazier · 2017-02-06T19:26:02Z

lib/plugins/docker/index.js

  getPattern() {
    return preset('Docker');
  }

  parseBlob(blob) {
-    insertLink(blob.el, DOCKER_FROM, {
-      resolver: 'dockerImage',
+    insertLink(this, blob.el, DOCKER_FROM, {


Instead of passing the plugin as the first argument to insertLink, what if we just put it into the object literal, alongside image:

insertLink(blob.el, DOCKER_FROM, { pluginName: this.constructor.name, image: '$1', });

This way, insertLink won't have to pass an extra argument to replace and buildDataAttr at all. Note that doing this would invalidate my comment at https://github.com/OctoLinker/browser-extension/pull/274/files#r99657840, since the extra argument will no longer be present.

The idea of passing the plugin to insertLink was to avoid duplication of pluginName: this.constructor.name across all plugins. To avoid conflicts with other prop key names passed to insertLink the name is prefixed with an underscore.

Maybe it's not the best idea, since it hides the actual behaviour and makes it just more complicated. Thanks for pointing me in the right direction.

That's why I believe and love code reviews 👍

Yeah, I was thinking about the duplication as well, when I was writing the comment. It's not ideal, but I think it's worth the gains in clarity that putting it in the map gives.

If you'd like, you could instead pass plugin: this to insertLink, and insertLink could delete the plugin key and add a pluginName key, to avoid duplicating .constructor.name, but I'm not sure if that's better...

I definitely agree on loving code reviews 👍

I went with the map solution for simplicity.

josephfrazier · 2017-02-06T19:27:05Z

lib/plugins/python/index.js

  getPattern() {
    return preset('Python');
  }

  parseBlob(blob) {
-    insertLink(blob.el, PYTHON_IMPORT, {
-      resolver: 'pythonUniversal',
+    insertLink(this, blob.el, PYTHON_IMPORT, {


(same comment as https://github.com/OctoLinker/browser-extension/pull/274/files#r99663118)

stefanbuck · 2017-02-06T22:24:56Z

Thanks for the great feedback. I'll follow up on your naming suggestions.

Regarding the refactoring. Do you think it makes sense to open a refactor PR for every single plugin? If we do that, we have to make it backwards compatible in order to keep the other plugins working. If you agree, I would like to open one big PR where every plugin refactoring is a commit. This should make it easier to review.

josephfrazier · 2017-02-07T00:57:19Z

I think it'd be much better to have a single PR with one plugin per commit, as you've described. Making separate PRs doesn't seem like it's worth the effort of maintaining backwards-compatibility. I look forward to seeing the updates!

stefanbuck · 2017-02-08T00:00:26Z

test/plugins/docker.test.js

@@ -1,38 +1,38 @@
 import assert from 'assert';
-import dockerImage from '../../lib/resolver/docker-image.js';
+import dockerImage from '../../lib/plugins/docker';


For simplicity and keeping the PR small as possible, I just fixed the broken tests. I'll polish all plugin tests in a separate PR.

stefanbuck · 2017-02-21T23:50:55Z

lib/plugins/bower-manifest.js

+      gitUrl({ target }),
+      githubShorthand({ target }),
+    ];
+  }


@josephfrazier what do you think about handling the different resolver types in one handler? It feels a bit inconvenient to me. In the old implementation this case was handled in a better way.

One possible option would be to split the plugin by features. In this case one for linking the dependencies and another which is responsible for linking the main field. I'm not sure if this makes thing easier or more complicated.

Are you talking about the calls to JavaScriptPlugin.resolve, liveResolverQuery, gitUrl, and githubShorthand being made explicitly, rather than implicitly like they are here and here? That doesn't really seem like a problem to me, but perhaps I'm misunderstanding the question. Splitting the Bower plugin into multiple other plugins sounds like it would just make things more complicated, especially if it's done as part of this refactor, rather than as a separate change.

I'm talking about the if statements in the resolve function. In my current implementation, the type attributes define which resolver is used. Without knowing how the click-handler works, I would assume that every insertLink can have his own click handler. What do you think? Maybe I'm over-thinking it.

Let's ignore this for now. It's not really important

Yeah, I think we can worry about that later if needed :)

stefanbuck · 2017-02-21T23:56:08Z

I'm almost complete with this one. There is one outstanding question regarding the resolve handler function. Besides that I'm almost through. I'll do some minor improvements + the remaining code review comments later this week.

stefanbuck · 2017-02-26T21:29:33Z

@josephfrazier can you help with this? Disabling the cache didn't helped.

Downloading googlechrome 64 bit
  from 'https://dl.google.com/tag/s/dl/chrome/install/googlechromestandaloneenterprise64.msi'
Progress: 100% - Completed download of C:\Users\appveyor\AppData\Local\Temp\1\chocolatey\GoogleChrome\56.0.2924.87\googlechromestandaloneenterprise64.msi (48.26 MB).
Download of googlechromestandaloneenterprise64.msi (48.26 MB) completed.
Error - hashes do not match. Actual value was '1450DE87F0289CE725DF1C138BBA2666304FB94BF4476B03C06D058C999D4805'.
ERROR: Checksum for 'C:\Users\appveyor\AppData\Local\Temp\1\chocolatey\GoogleChrome\56.0.2924.87\googlechromestandaloneenterprise64.msi' did not meet 'c47ff551ff8b251268905cd87fb299959e6bcb3640a3e60085a3a7dbf176845f' for checksum type 'sha256'. Consider passing --ignore-checksums if necessary.
The install of googlechrome was NOT successful.
Error while running 'C:\ProgramData\chocolatey\lib\GoogleChrome\tools\chocolateyInstall.ps1'.
 See log for details.
Chocolatey installed 0/1 packages. 1 packages failed.
 See the log for details (C:\ProgramData\chocolatey\logs\chocolatey.log).
Failures
 - googlechrome (exited -1) - Error while running 'C:\ProgramData\chocolatey\lib\GoogleChrome\tools\chocolateyInstall.ps1'.
 See log for details.
Did you know the proceeds of Pro (and some proceeds from other

josephfrazier · 2017-02-26T21:42:27Z

Hmm, that's a weird one. I just searched for the error and it looks like other people are seeing it too: https://chocolatey.org/packages/GoogleChrome#comment-3171878283

I don't really know why this would be happening, but it seems fine to add --ignore-checksums to the install command, considering it's being downloaded over HTTPS from Google.

stefanbuck · 2017-02-27T16:21:02Z

@josephfrazier can I merge it by myself or is there anything left from your side?

josephfrazier · 2017-02-27T16:49:27Z

This looks good to me. Excellent work on this! I'll let you do the honors of merging 👍

stefanbuck · 2017-02-27T16:55:13Z

😄 🎉

stefanbuck added Do not merge to-discuss labels Feb 1, 2017

stefanbuck requested a review from josephfrazier February 1, 2017 23:29

josephfrazier requested changes Feb 6, 2017

View reviewed changes

stefanbuck force-pushed the clickhandler-refactoring branch 5 times, most recently from 141eee7 to 04b9ae2 Compare February 7, 2017 23:56

stefanbuck commented Feb 8, 2017

View reviewed changes

stefanbuck added Work in progress and removed to-discuss labels Feb 8, 2017

stefanbuck force-pushed the clickhandler-refactoring branch from 04b9ae2 to ee99bba Compare February 9, 2017 23:45

stefanbuck force-pushed the clickhandler-refactoring branch 2 times, most recently from ea15c73 to e24c1dd Compare February 21, 2017 23:40

stefanbuck commented Feb 21, 2017

View reviewed changes

stefanbuck force-pushed the clickhandler-refactoring branch 4 times, most recently from 63a85f7 to eb7b6c3 Compare February 25, 2017 09:27

stefanbuck removed Do not merge Work in progress labels Feb 25, 2017

stefanbuck force-pushed the clickhandler-refactoring branch from 7d3ef18 to eb7b6c3 Compare February 26, 2017 21:28

stefanbuck added 14 commits February 27, 2017 07:30

Declare resolver function in plugin instead of a separate file

faaffd1

Refactor Docker plugin

5f53f5c

Refactor Python plugin

25e3516

Refactor Rust plugin

8072542

Refactor Vim plugin

2aced37

Refactor Ruby plugin

214b566

Refactor Go plugin

9b596c9

Refactor .Net Core plugin

8ac2754

Refactor Rubygems plugin

d099256

Refactor Homebrew plugin

12aba8c

Refactor Composer plugin

1d30ef3

Refactor JavaScript plugin

22e1946

Refactor TypeScript plugin

6a6ac80

Refactor bower pluign

8904406

stefanbuck force-pushed the clickhandler-refactoring branch 2 times, most recently from 705e742 to 67a2726 Compare February 27, 2017 06:45

Refactor npm plugin

67a2726

josephfrazier approved these changes Feb 27, 2017

View reviewed changes

stefanbuck merged commit 713cb22 into OctoLinker:master Feb 27, 2017

stefanbuck deleted the clickhandler-refactoring branch February 27, 2017 16:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Click handler refactoring proposal #274

Click handler refactoring proposal #274

stefanbuck commented Feb 1, 2017 •

edited

Loading

josephfrazier commented Feb 2, 2017

stefanbuck commented Feb 2, 2017

josephfrazier left a comment •

edited

Loading

josephfrazier Feb 6, 2017

josephfrazier Feb 6, 2017

josephfrazier Feb 6, 2017

josephfrazier Feb 6, 2017

josephfrazier Feb 6, 2017

josephfrazier Feb 6, 2017

josephfrazier Feb 6, 2017

josephfrazier Feb 6, 2017

stefanbuck Feb 6, 2017 •

edited

Loading

josephfrazier Feb 7, 2017

stefanbuck Feb 8, 2017

josephfrazier Feb 6, 2017

stefanbuck commented Feb 6, 2017

josephfrazier commented Feb 7, 2017

stefanbuck Feb 8, 2017

stefanbuck Feb 21, 2017

josephfrazier Feb 22, 2017

stefanbuck Feb 22, 2017

stefanbuck Feb 25, 2017

josephfrazier Feb 26, 2017

stefanbuck commented Feb 21, 2017 •

edited

Loading

stefanbuck commented Feb 26, 2017 •

edited

Loading

josephfrazier commented Feb 26, 2017

stefanbuck commented Feb 27, 2017

josephfrazier commented Feb 27, 2017

stefanbuck commented Feb 27, 2017

		@@ -4,13 +4,25 @@ import preset from '../../pattern-preset';

		export default class Docker {

		static clickHandler({ image }) {


		export default class Python {

		static clickHandler({ path, target }) {

Click handler refactoring proposal #274

Click handler refactoring proposal #274

Conversation

stefanbuck commented Feb 1, 2017 • edited Loading

TODO

josephfrazier commented Feb 2, 2017

stefanbuck commented Feb 2, 2017

josephfrazier left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stefanbuck Feb 6, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stefanbuck commented Feb 6, 2017

josephfrazier commented Feb 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stefanbuck commented Feb 21, 2017 • edited Loading

stefanbuck commented Feb 26, 2017 • edited Loading

josephfrazier commented Feb 26, 2017

stefanbuck commented Feb 27, 2017

josephfrazier commented Feb 27, 2017

stefanbuck commented Feb 27, 2017

stefanbuck commented Feb 1, 2017 •

edited

Loading

josephfrazier left a comment •

edited

Loading

stefanbuck Feb 6, 2017 •

edited

Loading

stefanbuck commented Feb 21, 2017 •

edited

Loading

stefanbuck commented Feb 26, 2017 •

edited

Loading