search

fiatjaf / module-linker

browse modules by clicking directly on "import" statements on GitHub
https://module-linker.fiatjaf.com/
MIT License
251 stars 25 forks source link

Linker might run multiple times #55

Closed SidneyNemzer closed 4 years ago

SidneyNemzer commented 4 years ago

It looks like the 'don't run more than once' protection is broken because Github changed their markup?

This line injects an element to stop the content script from running multiple times but I think it doesn't work because the element it injects into does not exist (or maybe it doesn't always exist).

In this screenshot, notice that there are multiple dots next to the imports, and the <span id="module-linker-done"> doesn't exist in the page.

image

fiatjaf commented 4 years ago

No. I think it was always that way, unrelated to GitHub changes. I didn't know it didn't exist, however. I thought it was a race condition that only happened in Firefox, but apparently it isn't.

Can you conjure up a fix for this?

SidneyNemzer commented 4 years ago

Oh interesting. If I have some time this weekend I'll debug a bit more and see what I come up with.

SidneyNemzer commented 4 years ago

So after a bit of investigation, it looks like the js-repo-pjax-container ID is not always added to the main. When navigating directly to a file, Github does not add the ID, but navigating to a folder or the root of the repo will get the ID.

Test this for yourself using curl https://github.com/fiatjaf/module-linker | grep main and curl https://github.com/fiatjaf/module-linker/blob/master/package.json | grep main. The first should show a main with the js-repo-pjax-container ID, the second does not.

Normally, this is fine for module linker because you can't navigate directly between files without going to a folder. But I'm using Octotree, which can use some kind of PJAX request from any page. If I go directly to a file, then use Octotree to navigate to another, module-linker links things more than once because the ID is missing.

This is pretty easy to fix, by replacing everything selecting #js-repo-pjax-container with main. In my brief testing, this fixed the problem and did not cause any other issues. (It also allows module-linker to PJAX request when you navigate directly to a file and click a linked module from the same repo, which it cannot right now).

I can open a PR if you think this is a good fix!

fiatjaf commented 4 years ago

I think it is a good idea and you've solved the biggest issue this project has ever faced. Please open a PR.