Gather urls while initializing extension #448
Conversation
| @@ -1,153 +0,0 @@ | |||
| import assert from 'assert'; | |||
There was a problem hiding this comment.
In another PR I'll get rid of click-handler at all. I think it's okay to not spent time on fixing the test. I did manual QA on this anyway.
| $body.undelegate(LINK_SELECTOR, 'click', onClick); | ||
| $body.undelegate(LINK_SELECTOR, 'mouseup', onClick); | ||
|
|
||
| $body.delegate(LINK_SELECTOR, 'click', onClick); | ||
| $body.delegate(LINK_SELECTOR, 'mouseup', onClick); | ||
|
|
||
| pluginManager = _pluginManager; | ||
| matches = _matches; |
There was a problem hiding this comment.
For simplicity I tweaked the click-handler according to my needs. As mentioned above, it will go away soon.
| link.dataset[key] = data[key]; | ||
| } | ||
| } | ||
| }); |
There was a problem hiding this comment.
We used data-attributes to hold a reference to the plugin and other plugin related informations. Now we pass the plugin itself down which makes things a lot easier.
| export default function(el, regex, mapping, captureGroup = '$1') { | ||
| if (!(el instanceof HTMLElement)) { | ||
| throw new Error('must be called with a DOM element'); | ||
| export default function(blob, regex, plugin, meta = {}) { |
There was a problem hiding this comment.
meta can be used to pass additional informations down to the resolver function. Right now this is only used by the bower and npm manifest plugin to distinguish between different resolver behaviour see https://github.com/stefanbuck/OctoLinker/blob/850801fd5e2cfd91db428e3f0173956c03c756aa/packages/plugin-npm-manifest/index.js#L13-L25
stefanbuck
force-pushed
the
call-resolvers-upfront
branch
from
850801f to
a2348df
Compare
| } | ||
|
|
||
| export default { | ||
| name: 'NpmManifest', | ||
|
|
||
| resolve({ target, path, type }) { | ||
| resolve(path, values, { type }) { |
There was a problem hiding this comment.
The resolve function receives the RegExp result as the second argument in order to get rid of the captured group ($1, $2 ...) magic. In most of the cases you are interested the first value only.
In some rare cases like this npm plugin, you need to differentiate based on the value the next step e.g. calling resolver with different values.
|
@josephfrazier I added a few comments, I hope this helps reviewing this PR. Please don't hesitate to ask if something is still unclear. |
|
I won't say I completely understand all the changes yet, but I don't have to ;). I did a smoke-test and it seems fine, so feel free to merge. I'm not sure whether you want to squash or rebase, so I'll leave it up to you. |
|
I can imagine, that looking at all this changes ins't easy. I tried to keep it as simple as possible. Thanks for the review |
This is another PR down the road to resolve all links on start-up. With this PR we still lazy load the url on click, but upfront we gather all possible urls and cache the results on start-up. I changed the signature of
plugin.resolve()for consistency and readability which leads to a bigger PR size. All critical changes are made in the first commit. I hope it's not to bad to review.